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

Add Enzyme reverse rules #110

Merged
merged 17 commits into from
Jul 31, 2024
Merged

Add Enzyme reverse rules #110

merged 17 commits into from
Jul 31, 2024

Conversation

wsmoses
Copy link
Contributor

@wsmoses wsmoses commented Jul 26, 2024

No description provided.

test/runtests.jl Outdated Show resolved Hide resolved
Copy link
Member

@giordano giordano left a comment

Choose a reason for hiding this comment

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

Minor whitespace changes

test/Project.toml Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
@stevengj
Copy link
Member

Missing the new extension files?

wsmoses and others added 2 commits July 26, 2024 14:46
Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
@stevengj stevengj marked this pull request as draft July 26, 2024 18:46
wsmoses and others added 3 commits July 26, 2024 14:47
Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
@wsmoses
Copy link
Contributor Author

wsmoses commented Jul 26, 2024

Whoops -- added actual file

Copy link

codecov bot commented Jul 26, 2024

Codecov Report

Attention: Patch coverage is 35.93750% with 41 lines in your changes missing coverage. Please review.

Project coverage is 93.43%. Comparing base (d5bbb69) to head (5a1f362).
Report is 7 commits behind head on master.

Files Patch % Lines
ext/QuadGKEnzymeExt.jl 28.07% 41 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #110      +/-   ##
==========================================
- Coverage   98.53%   93.43%   -5.10%     
==========================================
  Files           7        8       +1     
  Lines         682      762      +80     
==========================================
+ Hits          672      712      +40     
- Misses         10       50      +40     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wsmoses
Copy link
Contributor Author

wsmoses commented Jul 26, 2024

@stevengj looks like this all passes, except there's an installation issue on 1.2 for the tests.

That feels really old so I'm wondering if its just easiest to bump that to 1.6 as the LTS?

@giordano
Copy link
Member

giordano commented Jul 26, 2024

Note that proper package extensions are in Julia v1.9 only, for previous versions you can have a hack which involves depending unconditionally on the other package (Enzyme in this case) or using Requires, but if you want to bump the minimum version you may jump directly to v1.9.

@stevengj
Copy link
Member

stevengj commented Jul 26, 2024

I have no objection to bumping the required version to 1.9. I just tagged a release (JuliaRegistries/General#111854) so that 1.2 users will have access to fixes/features up to this point.

(Basically, my philosophy is to support older Julia releases until it becomes frustratingly inconvenient. Not having package extensions for differentiation rules definitely falls under "frustratingly inconvenient" to me.)

wsmoses and others added 3 commits July 26, 2024 16:36
Co-authored-by: Steven G. Johnson <stevenj@mit.edu>
@wsmoses wsmoses marked this pull request as ready for review July 30, 2024 00:10
@stevengj
Copy link
Member

Can we increase the codecov of the additions?

@wsmoses
Copy link
Contributor Author

wsmoses commented Jul 30, 2024 via email

@wsmoses
Copy link
Contributor Author

wsmoses commented Jul 30, 2024 via email

ext/QuadGKEnzymeExt.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
@stevengj
Copy link
Member

Okay, should be good to merge?

@wsmoses
Copy link
Contributor Author

wsmoses commented Jul 30, 2024

sgtm, but I don't have merge rights

@stevengj stevengj merged commit b8a65b4 into JuliaMath:master Jul 31, 2024
8 of 10 checks passed
@wsmoses wsmoses deleted the enz branch July 31, 2024 00:46
@stevengj
Copy link
Member

I'm planning to make a new release of QuadGK, hopefully it's fine to include this in its current state?

Without EnzymeAD/Enzyme.jl#1692 it's not as useful as I'd like, but it still seems better than nothing.

@wsmoses
Copy link
Contributor Author

wsmoses commented Aug 31, 2024

Yeah I think that's fine.

And yes that remains on my todo list (but I ended up spending a week fixing a segfault in julia itself from an issue someone hit since that's a lot worse of an error to have JuliaLang/julia#55397)

@stevengj
Copy link
Member

stevengj commented Aug 31, 2024

At some point it would also be good to fix the hack:

Enzyme.Compiler.recursive_add(b, b, x->(a-1)*x, guaranteed_nonactive)::CV

to compute a*b. The problem with this is that it is susceptible to catastrophic cancellation if $|a| \ll 1$ … it would be better to have a simple multiplication here (or at least generalize recursive_add to support a map for both operands, or have a special zero vector you can pass for one of the operands).

Better yet, implement something like ClosureVector in Enzyme itself, since any differentiation through a closure will run into a similar need.

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.

3 participants