Skip to content

Support ChainRulesCore v1 and ChainRulesTestUtils v1 #27

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 7 commits into from
Oct 3, 2021

Conversation

sethaxen
Copy link
Collaborator

@sethaxen sethaxen commented Aug 3, 2021

Supersedes #25, #26, and #28

@sethaxen
Copy link
Collaborator Author

sethaxen commented Aug 3, 2021

The tests currently fail because ChainRulesTestUtils.test_approx doesn't know what to do with the QDHT type (see example). @mzgubic, what's the right way in ChainRulesTestUtils to use test_frule and test_rrule on constructors?

@mzgubic
Copy link
Contributor

mzgubic commented Aug 4, 2021

The usage looks right to me.

The error we are seeing is fixed by defining Base.:(==) for QDHT.

The next error comes from Tangent{QDHT} and QDHT not having the same length when to_vecd. This could be fixed by JuliaDiff/FiniteDifferences.jl#188 (but doing that might mess up some other ChainRules tests - I haven't checked yet.)

Doing both fixes still fails the tests, namely the finite differencing thinks the R is actually differentiable. I don't know anything about Hankel so not sure if that makes sense? In case the derivative exists but hasn't been worked out yet @not_implemented macro can be used.

My suggestion would be to use the existing tests and open an issue to use test_f/rrule

@sethaxen sethaxen closed this Oct 3, 2021
@sethaxen sethaxen reopened this Oct 3, 2021
@sethaxen sethaxen merged commit 9e33eb5 into master Oct 3, 2021
@sethaxen sethaxen deleted the chainrulesv1 branch October 3, 2021 10:59
@sethaxen sethaxen mentioned this pull request Oct 3, 2021
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.

2 participants