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

Changing Distances adjoints to ChainRules syntax #923

Closed
wants to merge 17 commits into from

Conversation

theogf
Copy link

@theogf theogf commented Mar 22, 2021

Using @adjoint disable the overloading of rrule see : JuliaDiff/ChainRulesCore.jl#239 (comment)
It makes it impossible to define new rules for different metrics.
I am aware that the long-term plan is to move out the rules to the respective packages but in the mean-time it would be nice for it to be solved (it blocks our development in JuliaGaussianProcesses/KernelFunctions.jl#208).

@DhairyaLGandhi
Copy link
Member

Lgtm for the most part.

@DhairyaLGandhi
Copy link
Member

@adjoint does take precedence over rrule, so you could also define it via the macro to change it later. Although I would ask what the use case for that would be?

@theogf
Copy link
Author

theogf commented Mar 22, 2021

Sorry, I made a mistake: trying JuliaGaussianProcesses/KernelFunctions.jl#208 with this branch does not seem to solve the problem.

Also precompiling this patch returns a surprising warning:

┌ Warning: Error requiring `Distances` from `Zygote`
│   exception =
│    LoadError: error in method definition: function ChainRulesCore.rrule must be explicitly imported to be extended
...

@DhairyaLGandhi
Copy link
Member

Yeah, so we usually move the adjoints over, but since ChainRules is a dep, you could add the import

@willtebbutt
Copy link
Member

@theogf I know you've resolved this on your end now, but would you be up for pushing this PR through anyway? Seems like something that we should be doing throughout Zygote when the opportunity arises.

@devmotion
Copy link
Collaborator

It seems the adjoint definitions have to be updated to the ChainRules-syntax? In particular, I guess nothing should be replaced with the ChainRules-equivalents.

@theogf
Copy link
Author

theogf commented Mar 22, 2021

I officially don't know how to import rrule correctly via Requires...

@theogf
Copy link
Author

theogf commented Mar 26, 2021

I am not sure the failing checks are related to this PR...

@willtebbutt
Copy link
Member

Doesn't look related to me. @theogf I reckon this is basically good to go after a patch bump. I'll bors it after that unless @DhairyaLGandhi has any objections?

@DhairyaLGandhi
Copy link
Member

DhairyaLGandhi commented Mar 26, 2021

There was a merge conflict, otherwise lgtm

Project.toml Outdated Show resolved Hide resolved
src/lib/distances.jl Outdated Show resolved Hide resolved
src/lib/distances.jl Outdated Show resolved Hide resolved
src/lib/distances.jl Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

I think it would also be good to add some tests with ChainRulesTestUtils.test_rrule.

src/lib/distances.jl Outdated Show resolved Hide resolved
src/lib/distances.jl Outdated Show resolved Hide resolved
src/lib/distances.jl Outdated Show resolved Hide resolved
src/lib/distances.jl Outdated Show resolved Hide resolved
src/lib/distances.jl Outdated Show resolved Hide resolved
src/lib/distances.jl Outdated Show resolved Hide resolved
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
@mcabbott mcabbott added the ChainRules adjoint -> rrule, and further integration label Jul 4, 2022
@CarloLucibello
Copy link
Member

needs a rebase, otherwise seems a sensible change

@devmotion
Copy link
Collaborator

I wonder if possibly maintainers of Distances who didn't want to add a dependency on ChainRulesCore might be fine with adding it as a weak dependency?

src/lib/distances.jl Outdated Show resolved Hide resolved
@ToucheSir
Copy link
Member

CI is complaining about a \Delta not being defined, but it seems like it is?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ChainRules adjoint -> rrule, and further integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants