Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add basic complex functions, fix 2-arg atan #64

Merged
merged 5 commits into from
Aug 31, 2019
Merged

Add basic complex functions, fix 2-arg atan #64

merged 5 commits into from
Aug 31, 2019

Conversation

simeonschaub
Copy link
Member

No description provided.

ararslan
ararslan previously requested changes Jun 21, 2019
View reviewed changes
test/rules/base.jl Outdated Show resolved Hide resolved
test/rules/base.jl Outdated Show resolved Hide resolved
test/rules/base.jl Outdated Show resolved Hide resolved
@simeonschaub
Copy link
Member Author

@ararslan I've implemented your requested changes. Do you agree with the way isapprox behaves now?

@simeonschaub simeonschaub mentioned this pull request Jun 21, 2019
Closed
@ararslan
Copy link
Member

I think the isapprox overloads should stay in the test utilities.

@simeonschaub
Copy link
Member Author

I think the isapprox overloads should stay in the test utilities.

Yes, probably good not to overcomplicate the definition of AbstractDifferentials.

ararslan
ararslan reviewed Jun 21, 2019
View reviewed changes
src/rules/base.jl Outdated Show resolved Hide resolved
@simeonschaub
Copy link
Member Author

Anything still preventing this from getting merged?

@ararslan ararslan dismissed their stale review July 3, 2019 19:02

Requested changes implemented

@ararslan ararslan requested a review from willtebbutt July 3, 2019 19:02
@oxinabox
Copy link
Member

@simeonschaub sorry for not attending to these earlier, if we had have I think some of the problems @shashi has been running into would already be fixed.

I am away for next 2 weeks. But hopefully someone else can attend to these while I am gone.

@nickrobinson251
Copy link
Contributor

I think this just needs rebasing to resolve the conflicts?

@simeonschaub
Copy link
Member Author

Something went wrong...

@oxinabox
Copy link
Member

@simeonschaub are you using merge?
You want to be using rebase.
So you don't get a bunch of extra commits in your PR from changes in master.

I am not great at explaining it, but googling
"How to rebase my PR" hopefully has explaination.

@simeonschaub
Copy link
Member Author

@oxinabox Yes, that was probably it. I now just force-pushed it as one commit

nickrobinson251
nickrobinson251 reviewed Aug 29, 2019
View reviewed changes
test/rulesets/Base/base.jl Outdated Show resolved Hide resolved
src/rulesets/Base/base.jl Show resolved Hide resolved
@simeonschaub
Copy link
Member Author

It seems like some of the complex tests need larger tolerances

@simeonschaub
Copy link
Member Author

@oxinabox Would it be possible to tag a new release for ChainRulesCore, because that seems to be what is breaking the Wirtinger tests right now?

@oxinabox
Copy link
Member

Tagged, waiting for it to merge.
JuliaRegistries/General#3056

@oxinabox
Copy link
Member

I have restarted the CI now that the never version of ChainRulesCore has been tagged.

@simeonschaub
Copy link
Member Author

Should be ready to merge now

nickrobinson251
nickrobinson251 approved these changes Aug 31, 2019
View reviewed changes
Copy link
Contributor

@nickrobinson251 nickrobinson251 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nickrobinson251 nickrobinson251 merged commit 63fe051 into JuliaDiff:master Aug 31, 2019
@simeonschaub simeonschaub deleted the complex_utils branch August 31, 2019 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ararslan ararslan ararslan left review comments

@nickrobinson251 nickrobinson251 nickrobinson251 approved these changes

@willtebbutt willtebbutt Awaiting requested review from willtebbutt

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@simeonschaub @ararslan @oxinabox @nickrobinson251