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

Enzyme: try bump without tuple changes #3618

Merged
merged 25 commits into from
Jun 27, 2024
Merged

Enzyme: try bump without tuple changes #3618

merged 25 commits into from
Jun 27, 2024

Conversation

wsmoses
Copy link
Collaborator

@wsmoses wsmoses commented Jun 8, 2024

Requires EnzymeAD/Enzyme.jl#1485 to release, which itself requires JuliaRegistries/General#108556

Perhaps means we don't change the other varargs stuff, but we'll have to wait and see.

@wsmoses wsmoses requested a review from glwagner June 8, 2024 20:06
@wsmoses wsmoses closed this Jun 8, 2024
@wsmoses wsmoses reopened this Jun 8, 2024
@wsmoses wsmoses closed this Jun 8, 2024
@wsmoses wsmoses reopened this Jun 8, 2024
@wsmoses wsmoses closed this Jun 8, 2024
@wsmoses wsmoses reopened this Jun 8, 2024
@wsmoses wsmoses closed this Jun 9, 2024
@wsmoses wsmoses reopened this Jun 9, 2024
@wsmoses wsmoses closed this Jun 9, 2024
@wsmoses wsmoses reopened this Jun 9, 2024
autodiff(Enzyme.Reverse,
set_initial_condition!,
Duplicated(model, dmodel),
Active(1.0))
Copy link
Member

Choose a reason for hiding this comment

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

some indendenting irregularities here and above

Comment on lines 117 to 119
include_right_boundaries = false,
reduced_dimensions = (),
location = nothing,
Copy link
Member

Choose a reason for hiding this comment

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

fix the indenting here

@glwagner
Copy link
Member

seems simple enough, just a few cosmetic comments. Does shadowp stand for "primal_shadow"?

Project.toml Outdated
@@ -11,6 +11,7 @@ CubedSphere = "7445602f-e544-4518-8976-18f8e8ae6cdb"
Dates = "ade2ca70-3891-5945-98fb-dc099432e06a"
Distances = "b4f34e82-e78d-54a5-968a-f98e89d6e8f7"
DocStringExtensions = "ffbed154-4ef7-542d-bbb7-c09d3a79fcae"
Enzyme = "7da242da-08ed-463a-9acd-ee780be4f1d9"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be in [extras] if it is just a testdep

@wsmoses
Copy link
Collaborator Author

wsmoses commented Jun 27, 2024

@glwagner @jlk9 with a bunch more fixes pushed into Enzyme itself, this now fully passes the Enzyme (cpu) tests so I think we should be good to merge!

@wsmoses wsmoses closed this Jun 27, 2024
@wsmoses wsmoses reopened this Jun 27, 2024
@wsmoses
Copy link
Collaborator Author

wsmoses commented Jun 27, 2024

@navidcy @glwagner this should superceded all previous enzyme prs (including compatbwlper), which I've gone ahead and closed just now.

Some non enzyme tests had a nondeterministic ci failure about a directory not being empty, but otherwise all pass (incl enzyme tests).

Once landed we should add a lot more integration test cases like @jlk9 your flux boundary

@glwagner glwagner merged commit 13dabfb into main Jun 27, 2024
46 checks passed
@glwagner glwagner deleted the enzbump branch June 27, 2024 20:54
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.

None yet

4 participants