Skip to content

use rrules even when all the arguments are types #1006

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.

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

Merged
merged 3 commits into from
Jun 24, 2021
Merged

Conversation

oxinabox
Copy link
Member

Fixes this one of SciML/SciMLBase.jl#69 (comment)
(cc @ChrisRackauckas)
Problem was that our resolving order was:

  1. ZygoteRules definition (directly overloading _pullback
  2. Check if should be ignored based on types of input
  3. ChainRules definition
  4. generate code via decomposition.

But what it should have been, and what this PR changes it to is:

  1. ZygoteRules definition (directly overloading _pullback
  2. ChainRules definition
  3. Check if should be ignored based on types of input
  4. generate code via decomposition.

Also I think the new code is clearer, all chainrules stuff is together, and all the stuff where Zygote is working it out is together.

@oxinabox
Copy link
Member Author

@ChrisRackauckas can you click approve?

@ChrisRackauckas
Copy link
Member

How much money do you have on you?

Copy link
Member

@DhairyaLGandhi DhairyaLGandhi left a comment

Choose a reason for hiding this comment

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

Otherwise is fine, I think. I'm a bit iffy on whether we should be leaning on CR since we might want to preferentially dispatch to methods in Zygote first for different reasons.

@oxinabox
Copy link
Member Author

preferentially dispatch to methods in Zygote first for different reasons.

This is after that has already happened before we get to this point.

Because @adjoint defines a _pullback that will get hit rather than hitting the generated function.

Co-authored-by: Dhairya Gandhi <[email protected]>
@oxinabox oxinabox merged commit b170521 into master Jun 24, 2021
@oxinabox oxinabox deleted the ox/typeonlyrrules branch June 24, 2021 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants