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

Differentiability tests for time stepping #656

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

Conversation

maximilian-gelbrecht
Copy link
Member

Work in progress

While doing so I restructered all the differentiability tests und put them in a separate folder.

@maximilian-gelbrecht maximilian-gelbrecht added the differentiability 🤖 Making the model differentiable via AD label Jan 9, 2025
@maximilian-gelbrecht maximilian-gelbrecht marked this pull request as ready for review January 9, 2025 17:14
@maximilian-gelbrecht maximilian-gelbrecht marked this pull request as draft January 9, 2025 17:15
@maximilian-gelbrecht

This comment was marked as resolved.

@maximilian-gelbrecht

This comment was marked as resolved.

@milankl

This comment was marked as resolved.

@maximilian-gelbrecht
Copy link
Member Author

maximilian-gelbrecht commented Jan 31, 2025

I found the issue regarding RingGrids 🎉

We were apperently missing definitions of Base.dataids in our RingGrids that help unalias mutable variables within structs. The docstring of that function does actually recommend that array wrapper structs do implement it, but it's not part of the public API.

@maximilian-gelbrecht
Copy link
Member Author

maximilian-gelbrecht commented Feb 3, 2025

Some problems with our code with Enzyme in Julia 1.11 that work in Julia 1.10 (see e.g. the CI report), a minimal example without SpeedyWeather posted there EnzymeAD/Enzyme.jl#2295

@maximilian-gelbrecht
Copy link
Member Author

I was able to fix the correctness tests for the high-level timestepping! tests now quite quickly. So it seems we can in fact differentiate through all models correctly!

There's still something wrong for the more detailed tests of the primitive equation model, but I assume that's just user error, as the more high-level tests are correct.

There's not so much remaining to do here. Enzyme in Julia 1.11 is a bit of an issue, but also partially outside of our control.

@milankl I am off work for a week, I think this is already a good time for a proper code review in the meanwhile

@maximilian-gelbrecht maximilian-gelbrecht marked this pull request as ready for review February 5, 2025 07:52
@@ -1,6 +1,7 @@
[deps]
CairoMakie = "13f3f980-e62b-5c42-98c6-ff1f3baf88f0"
Documenter = "e30172f5-a6a5-5a46-863b-614d45cd2de4"
Enzyme = "7da242da-08ed-463a-9acd-ee780be4f1d9"
Copy link
Member Author

@maximilian-gelbrecht maximilian-gelbrecht Feb 5, 2025

Choose a reason for hiding this comment

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

Adding Enzyme to the docs and actually running the example code, makes the whole CI process for the docs much longer. So long actually, the process timed out. We may also just hardcode the Enzyme example in the docs.

Copy link
Member

Choose a reason for hiding this comment

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

Happy with that for now! We can later check how to automate tests if that's currently unfeasible

@maximilian-gelbrecht
Copy link
Member Author

As an outlook: I did already try to differentiate through multiple time steps with run! as well. And this does seem to work (correctness not separately checked), but of course for any actually longer runs, naively differentiating through everything becomes unfeasible due to the memory usage. This is when one needs to think about checkpointing e.g. with Checkpointing.jl.

@maximilian-gelbrecht
Copy link
Member Author

I actually skip the test in the regular CI for Julia 1.11. I think that's fine for now. I'd rather merge this sooner than later, and continue work on differentiability in further PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
differentiability 🤖 Making the model differentiable via AD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants