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

Utilities for splitting and unsplitting mode objects #1979

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

gdalle
Copy link
Contributor

@gdalle gdalle commented Oct 17, 2024

Fix #1938

I can also add more tests for these mode modifiers in a separate PR, if you're interested

@@ -384,7 +384,7 @@ Subtype of [`Mode`](@ref) for split reverse mode differentiation, to use in [`au
- [`set_abi`](@ref)
- [`ReverseSplitModified`](@ref), [`ReverseSplitWidth`](@ref)
"""
struct ReverseModeSplit{ReturnPrimal,ReturnShadow,Width,RuntimeActivity,ModifiedBetween,ABI,Holomorphic,ErrIfFuncWritten,ShadowInit} <: Mode{ABI, ErrIfFuncWritten,RuntimeActivity} end
struct ReverseModeSplit{ReturnPrimal,ReturnShadow,RuntimeActivity,Width,ModifiedBetween,ABI,Holomorphic,ErrIfFuncWritten,ShadowInit} <: Mode{ABI, ErrIfFuncWritten,RuntimeActivity} end
Copy link
Member

Choose a reason for hiding this comment

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

Probably keep this the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As explained in this comment, this just fixes a typo that doesn't change functionality but makes the code harder to understand. Semantically, the runtime activity parameter comes before width in all the uses of this struct, even though they are "labelled" wrong here.

Copy link
Member

@wsmoses wsmoses left a comment

Choose a reason for hiding this comment

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

One more test then LGTM!

@codecov-commenter
Copy link

codecov-commenter commented Oct 17, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.46%. Comparing base (037dfed) to head (5dd5adc).
Report is 179 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1979      +/-   ##
==========================================
+ Coverage   67.50%   69.46%   +1.95%     
==========================================
  Files          31       35       +4     
  Lines       12668    14929    +2261     
==========================================
+ Hits         8552    10370    +1818     
- Misses       4116     4559     +443     

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

@gdalle
Copy link
Contributor Author

gdalle commented Oct 18, 2024

Silly question: do the tests of EnzymeCore ever run? Because that's where I put these new checks

@wsmoses
Copy link
Member

wsmoses commented Oct 19, 2024

....good question. Probably we should just add a CI job for EnzymeCore

@gdalle
Copy link
Contributor Author

gdalle commented Oct 23, 2024

See #2009

@gdalle
Copy link
Contributor Author

gdalle commented Oct 23, 2024

Since #2009 has been merged by @vchuravy, I have integrated the changes here and I think this one is good to go.

@gdalle
Copy link
Contributor Author

gdalle commented Oct 25, 2024

Is anything more needed on my end?

@gdalle
Copy link
Contributor Author

gdalle commented Oct 31, 2024

Gentle bump on this one @wsmoses

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.

Utilities to translate from ReverseMode to ReverseModeSplit
3 participants