search

JuliaMath / QuadGK.jl

adaptive 1d numerical Gauss–Kronrod integration in Julia
MIT License
272 stars 37 forks source link

Add Enzyme reverse rules #110

Closed wsmoses closed 2 months ago

stevengj commented 2 months ago

Missing the new extension files?

wsmoses commented 2 months ago

Whoops -- added actual file

codecov[bot] commented 2 months ago

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 :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

wsmoses commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

I have no objection to bumping the required version to 1.9. I just tagged a release (https://github.com/JuliaRegistries/General/pull/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.)

stevengj commented 2 months ago

Can we increase the codecov of the additions?

wsmoses commented 2 months ago

At the moment I’m not sure.

This could be due in part to the second function not being hit due to the broken test we added and Enzyme early failing and not calling the code. Im also not sure how Julia code conv is implemented so it may not recognize the code emitted by enzyme for the custom rule as coming from the actual rule.

On Tue, Jul 30, 2024 at 3:38 PM Steven G. Johnson @.***> wrote:

Can we increase the codecov of the additions?

— Reply to this email directly, view it on GitHub https://github.com/JuliaMath/QuadGK.jl/pull/110#issuecomment-2259073478, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJTUXFRH62BKG6AUGFEYN3ZO7TSNAVCNFSM6AAAAABLQ6FWNCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJZGA3TGNBXHA . You are receiving this because you authored the thread.Message ID: @.***>

wsmoses commented 2 months ago

Spoke with @vchuravy and confirmed that GPUCompjler doesn’t insert the code coverage intrinsics. So, the answer is no — and not because it isn’t run — but that the code coverage tool doesn’t know

On Tue, Jul 30, 2024 at 3:42 PM Billy Moses @.***> wrote:

At the moment I’m not sure.

This could be due in part to the second function not being hit due to the broken test we added and Enzyme early failing and not calling the code. Im also not sure how Julia code conv is implemented so it may not recognize the code emitted by enzyme for the custom rule as coming from the actual rule.

On Tue, Jul 30, 2024 at 3:38 PM Steven G. Johnson < @.***> wrote:

Can we increase the codecov of the additions?

— Reply to this email directly, view it on GitHub https://github.com/JuliaMath/QuadGK.jl/pull/110#issuecomment-2259073478, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJTUXFRH62BKG6AUGFEYN3ZO7TSNAVCNFSM6AAAAABLQ6FWNCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJZGA3TGNBXHA . You are receiving this because you authored the thread.Message ID: @.***>

stevengj commented 2 months ago

Okay, should be good to merge?

wsmoses commented 2 months ago

sgtm, but I don't have merge rights

stevengj commented 1 month ago

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

Without https://github.com/EnzymeAD/Enzyme.jl/issues/1692 it's not as useful as I'd like, but it still seems better than nothing.

wsmoses commented 1 month ago

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 https://github.com/JuliaLang/julia/pull/55397)

stevengj commented 1 month ago

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.