-
Notifications
You must be signed in to change notification settings - Fork 11
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 EnzymeCore
weakdep and an extension with a custom rule for the Levin transformation
#97
Merged
+1,127
−117
Merged
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
444db1a
Attempt at an extension/weakdep with an Enzyme rule. Not currently
17b90ca
Switch weakdep to EnzymeCore.
766cff5
Add 2023 to license.
6db401e
Add dummy test to trigger failure.
dbb3821
Switching to tagged releases of Enzyme and EnzymeCore to fix the
e2d1f41
Extension fixed, test passing.
fc1e4a7
Test framework in place, but I don't trust these Arb derivatives.
71334c4
Oops, for got to un-comment the other tests.
afcb934
Switch to simple fd for enzyme test. Rtol=1e-15 works for all but six
9c6d063
Update test README that shows how I generated the file
6f3445c
@heltonmc's nice convergence check that fixes the scalar evaluation and
3c30394
Power series Temme-like method for near-integer/integer orders that
0e687a1
Checkpoint commit with small tweaks. Still chasing down a small issue
232f566
near-int autodiff working, just with slightly less precision with 1e-8
e317d3a
Rolling back to the old power series.
1578b6f
Update README for test data generation
27d257f
Fix manual `sinpi` rule, but CI and compat to 1.9.
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
using EnzymeCore, Enzyme | ||
import Bessels.BesselFunctions: besselkx_levin | ||
import Bessels.BesselFunctions: besselk_power_series | ||
|
||
dbesselkx_dv(v, x) = autodiff(Forward, _v->besselkx_levin(_v, x, Val(30)), | ||
Duplicated, Duplicated(v, 1.0))[2] | ||
|
||
dbesselkx_dx(v, x) = autodiff(Forward, _x->besselkx_levin(v, _x, Val(30)), | ||
Duplicated, Duplicated(x, 1.0))[2] | ||
|
||
#= | ||
dbesselk_ps_dv(v, x) = autodiff(Forward, _v->besselk_power_series(_v, x), | ||
Duplicated, Duplicated(v, 1.0))[2] | ||
|
||
dbesselk_ps_dx(v, x) = autodiff(Forward, _x->besselk_power_series(v, _x), | ||
Duplicated, Duplicated(x, 1.0))[2] | ||
=# | ||
|
||
|
||
for line in eachline("data/besselk/enzyme/besselkx_levin_enzyme_tests.csv") | ||
(v, x, dv, dx) = parse.(Float64, split(line)) | ||
test_dv = dbesselkx_dv(v, x) | ||
test_dx = dbesselkx_dx(v, x) | ||
@test isapprox(dv, test_dv, rtol=5e-14) | ||
@test isapprox(dx, test_dx, rtol=5e-14) | ||
end | ||
|
||
#= | ||
for line in eachline("data/besselk/enzyme/besselk_power_series_enzyme_tests.csv") | ||
(v, x, dv, dx) = parse.(Float64, split(line)) | ||
test_dv = dbesselk_ps_dv(v, x) | ||
test_dx = dbesselk_ps_dx(v, x) | ||
#@test isapprox(dv, test_dv, rtol=5e-14) | ||
#@test isapprox(dx, test_dx, rtol=5e-14) | ||
|
||
end | ||
=# | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Any way we can get this fixed in enzyme itself?
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.
Also, this should use
sincospi
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.
I poked over at EnzymeAD/Enzyme.jl#443
I don’t think I know exactly how to solve that though
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.
As it turns out this is not the only problem. Something else in the generic power series is broken for
Enzyme
but notForwardDiff
. Leaving a summary comment below, one moment.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.
When you have time could post a specific example where this is broken? I will try to figure out what line is causing the issue even when separating out the sinpi.
These issues though are especially annoying......
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.
Ironically sincospi using Enzyme should be fine. I'm adding a pr for sinpi/cospi now which hopefully will be available in a few days.
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.
Also, though I'm solving in a different way from this PR (internal to Enzyme proper rather than Enzyme.jl custom rule), rules like this are welcome as PR's to Enzyme.jl
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.
Perfect! Thanks for looking at this. I'll change it over here once that is available.
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.
It occurs to me to mention here for people looking at this PR in the future that the problem was just the
sinpi
, but that I didn't understand how to properly writeEnzymeRules
and just needed to propagate thex.dval
in the derivative part of the returnedDuplicated
object. I didn't include tests for power series accuracy here because that will probably be a little bit of a project to get the last few digits, but once I fixed my custom rule that worked fine.@wsmoses, would you like me to make a PR with this rule in the meantime? If it is fixed and will be available in the next release, maybe not point unless you would make a more immediate release that has the custom rule. I'd be happy to try and make that PR if you want, but I understand if it isn't the most useful.
Sorry that this thread looks on a skim like Enzyme problems but was actually "Chris doesn't know how to write custom rules" problems.
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.
See EnzymeAD/Enzyme#1216. Hopefully that fixes this issue here and we can remove that part in the future.
P.s. I have the general
besselk
working now locally so hopefully we can get that merged soon and can test the general derivative cases.