-
-
Notifications
You must be signed in to change notification settings - Fork 37
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
Conversation
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.
Minor whitespace changes
Missing the new extension files? |
Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
Whoops -- added actual file |
Codecov ReportAttention: Patch coverage is
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. |
@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? |
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 |
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.) |
Can we increase the codecov of the additions? |
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
<#110 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJTUXFRH62BKG6AUGFEYN3ZO7TSNAVCNFSM6AAAAABLQ6FWNCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJZGA3TGNBXHA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
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
> <#110 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAJTUXFRH62BKG6AUGFEYN3ZO7TSNAVCNFSM6AAAAABLQ6FWNCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJZGA3TGNBXHA>
> .
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
|
Okay, should be good to merge? |
sgtm, but I don't have merge rights |
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. |
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) |
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 Better yet, implement something like |
No description provided.