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

Add a Reactant Extension #438

Merged
merged 10 commits into from
Mar 24, 2025
Merged

Add a Reactant Extension #438

merged 10 commits into from
Mar 24, 2025

Conversation

glwagner
Copy link
Member

There wiill likely be a few methods that need special treatment.

cc @giordano @wsmoses @simone-silvestri

@@ -108,7 +108,7 @@ end
downwelling_radiation = (; Qs, Qℓ)

# Estimate initial interface state
FT = eltype(grid)
FT = typeof(Tᵢ)
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's the difference here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Basically Ti is the correct choice here, because it is the type of the variables in InterfaceState (typically a floating point). The eltype of the grid, on the other hand, is more general, because it can either be a traced floating point or a constant floating point. The status of the grid eltype might change in the future (eg maybe we will refine the design so that it is always a floating point type). But regardless of how we decide to treat eltype, I think this line is expressing the correct pattern.

Copy link
Member Author

@glwagner glwagner Mar 22, 2025

Choose a reason for hiding this comment

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

Ok here is a better explanation:

Ti is the eltype of the arrays. This eltype is more general than just a floating point, because in order to transpile Oceananigans/ClimaOcean with Reactant, the eltype of arrays must be "marked" (also known as "traced") as something that can change while the computation is running. This distinguishes the array eltype from the eltype of variables which do not change during a computation (these we refer to as "constants").

Prior to Reactant we did not distinguish between data that can change vs stay the same. Or actually I would say that we more generally simply allow anything at all to change through extensive use of mutable struct. But with Reactant we come into the position of having to specifically state what is changed and what is not.

We can decide to define eltype(grid) as the "underlying" floating point type of an array eltype. I think this might be a valid choice.

However, we retain generality (and in a sense more modularity in the code) if we propagate the array eltypes locally, rather than attempting to always infer the "global eltype" from the grid. The local inference pattern is actually a better / more modular design. It allows us to take the optimal approach for grid eltype regardless of what kernels look like.

I didn't realize any of this until Reactant came around, so I think this is a welcome improvement to realize that local inference is the correct approach. There are a handful of changes sprinkled into Oceananigans that are similar btw.

@giordano
Copy link
Collaborator

giordano commented Mar 22, 2025

I'm working on fixing the issue on Windows.

Edit: Fix for making Reactant loadable on Windows available at EnzymeAD/Reactant.jl#1003, but that's not actually making the test fail here because precompilation failure isn't critical and Windows tests don't test Reactant, so all good here.

@glwagner
Copy link
Member Author

I'm gonna merge this, then move the reactant extension tests to github actions where I think they have a better chance at succeeding

@glwagner glwagner merged commit 51bfcf1 into main Mar 24, 2025
15 of 18 checks passed
@glwagner glwagner deleted the glw/reactant-ext branch March 24, 2025 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants