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.

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

make @non_differentiable use identical pullbacks when possible #679

Merged
merged 3 commits into from
May 31, 2024
Merged

make @non_differentiable use identical pullbacks when possible #679

merged 3 commits into from
May 31, 2024

Conversation

nsajko
Copy link
Contributor

@nsajko nsajko commented May 30, 2024

Fixes #678

@nsajko
Copy link
Contributor Author

nsajko commented May 30, 2024

The test failures are integration tests, with ChainRulesOverloadGeneration.jl and Diffractor.jl. The ChainRulesOverloadGeneration.jl test failures seem unrelated, while Diffractor.jl has lots of opaque closure-related failures, but these fail independently of this PR. So I think this is good for review.

@nsajko
Copy link
Contributor Author

nsajko commented May 31, 2024

The change is now improved and the diff much smaller.

function $(esc(propagator_name(primal_name, :pullback)))(@nospecialize(_))
return $(tup_expr)
end
NonDiffPullback($(tup_expr))
Copy link
Member

Choose a reason for hiding this comment

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

I guess even simpler would be

Suggested change
NonDiffPullback($(tup_expr))
Returns($(tup_expr))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That will change the method signature, though. Currently the pullback accepts a single argument, while Returns accepts any amount of arguments. Is that OK to change?

Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

Looks good to me, I think this is the simplest approach 🙂

Can you update the version number?

@devmotion devmotion merged commit b9ea5d7 into JuliaDiff:main May 31, 2024
23 of 25 checks passed
@nsajko nsajko deleted the non_differentiable_identically_same_pullback branch June 1, 2024 02:19
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.

@non_differentiable should use identical pullbacks when possible
2 participants