-
Notifications
You must be signed in to change notification settings - Fork 89
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
Add rules for evalpoly #190
Conversation
Co-authored-by: Lyndon White <[email protected]>
Until complex conventions are clarified and FiniteDifferences v0.10.0 is supported.
This PR is basically feature complete, and tests pass. It's still marked as WIP because:
|
This for some reason profiles much faster
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should now be good to go, the tests look reasonable so I am happy to see this merged once they pass
Yeah, the weird |
I just found a question on discourse asking why Zygote won't differentiate a polynomial created with using Zygote
using Polynomials
p=Polynomial([1.0,0.0,3.0,4.0])
@show derivative(p)(5.0)
@show gradient(p,5.0) This errors because we need a
Now
gives the right result. This might be due to the fact that the indexing for p_tup = Tuple(p.coeffs)
p[0] == p_tup[1] # true
p[1] == p_tup[2] # true
# and so forth |
Most of the pushforward and pullback forevalpoly
can be written as more calls toevalpoly
, which is very efficient.Edit: This is true in the scalar case but not the matrix. In the current implementation, both the matrix and scalar polynomials have forward and reverse rules on the same order as
evalpoly
itself.Currently all but one of the tests fail for various reasons. I think the testers in ChainRulesTestUtils don't like when one of the inputs is aAlso, I don't expect the complex tests to pass until this package uses FiniteDifferences v0.10.0.Tuple
.Edit: Oh, and I see that ChainRules doesn't test against 1.4 yet at all.
@oxinabox I also tested against FluxML/Zygote.jl#366 and may have hit a bug where Zygote doesn't like that the adjoint ofp
is aTuple
, but I'm not sure.Edit: Now that I'm using
Composite
, it works fine in Zygote.