Skip to content

Add Reactant correctness check for AtmosphereModel.#687

Open
dkytezab wants to merge 24 commits into
mainfrom
dkz/feature-reactant-correctness
Open

Add Reactant correctness check for AtmosphereModel.#687
dkytezab wants to merge 24 commits into
mainfrom
dkz/feature-reactant-correctness

Conversation

@dkytezab

@dkytezab dkytezab commented May 8, 2026

Copy link
Copy Markdown
Collaborator

Closes #683. Checking field equality before and after a single timestep.

@giordano giordano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't run only on the GPU jobs? Also, it may be easier to move all these tests in a dedicate directory (reactant/?) and then in

if filter_tests!(testsuite, args)
# Reactant compilation tests require --check-bounds=auto (Reactant/Enzyme
# limitation).
if !REACTANT_COMPAT
delete!(testsuite, "reactant_centered_compilation")
delete!(testsuite, "reactant_weno_compilation")
end
end
automatically filter out all the tests starting with reactant/. Edit: I went on and did that.

@giordano giordano added testing 🧪 reactant ☣ towards a differentiable earth labels May 8, 2026
@codecov

codecov Bot commented May 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

giordano added 2 commits May 8, 2026 22:34
ParallelTestRunner fixes the seed anyway.
@dkytezab

dkytezab commented May 8, 2026

Copy link
Copy Markdown
Collaborator Author

I believe that the errors here https://github.com/NumericalEarth/Breeze.jl/actions/runs/25581771362/job/75102082127?pr=687#step:11:536 are due primarily to EnzymeAD/Reactant.jl#2870. Basically, when we compile with Reactant, we alias the initial state rather than storing it as a copy, causing the RK3 substeps to be incorrect. This enters here

function store_initial_state!(model)
U⁰ = model.timestepper.U⁰
for (u⁰, u) in zip(U⁰, prognostic_fields(model))
parent(u⁰) .= parent(u)
end
return nothing
end

My workaround was instead to use a kernel to copy each of the prognostic fields, which I can implement now for the time being if that sounds good

There may also be an issue with maybe_prepare_first_time_step! that I need to investigate further.

@giordano

Copy link
Copy Markdown
Member

I'm temporarily checking out EnzymeAD/Reactant.jl#2984 by @Pangoraw to see how it goes here.

@giordano

Copy link
Copy Markdown
Member

There are still correctness errors: https://github.com/NumericalEarth/Breeze.jl/actions/runs/27677328615/job/81855741446?pr=687#step:14:1076

Error in testset topology=Periodic, Periodic, Bounded:
Test Failed at /__w/Breeze.jl/Breeze.jl/test/reactant/correctness.jl:105
  Expression: report_state("topology=$(label) — after  time_step!", vmodel, rmodel; rtol, atol)

Error in testset topology=Periodic, Bounded,  Bounded:
Test Failed at /__w/Breeze.jl/Breeze.jl/test/reactant/correctness.jl:105
  Expression: report_state("topology=$(label) — after  time_step!", vmodel, rmodel; rtol, atol)

@dkytezab

Copy link
Copy Markdown
Collaborator Author

I think there is an issue with maybe_prepare_first_time_step. Correct me if I am wrong, but i believe that otherwise the initial tendency is different for the different models.

@Pangoraw

Copy link
Copy Markdown
Collaborator

I think there is an issue with maybe_prepare_first_time_step

should we call first_time_step! in the correctness test so that initialization is the same for Reactant and vanilla models?

@giordano

Copy link
Copy Markdown
Member

Where are you looking at?

@giordano

Copy link
Copy Markdown
Member

Reactant tests are timing out again, the way they did before #782

@giordano

Copy link
Copy Markdown
Member

Given that also #774 is stuck in Reactant tests, I take something is up in post v0.2.266?

Comment thread .github/workflows/CI.yml Outdated
# concurrent jobs, some of them reaching a large memory usage. We reduce the
# number of jobs to avoid memory issues.
TEST_ARGS+=(--jobs=$(($(nproc) - 1)))
TEST_ARGS+=(--quickfail)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've been thinking for a while of setting --quickfail for all jobs, maybe I'll open a separate PR later to just do that

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I suspect that we could learn more from the failure with it since the timeout seems to prevent the buffered results from being shown.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In this case were jobs even time out that's really necessary, but in general it's a bit frustrating having to wait long time to find what's failing, which was my motivation for doing this.

I opened #798, elaborating the downside and its mitigation.

@Pangoraw Pangoraw Jun 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ok with quickfail, we actually catch the error (not a timeout)

2026-06-23T21:53:36.4406195Z   UNKNOWN: <unknown>:0: error: 'math.ctlz' op unsupported op for export to XLA
2026-06-23T21:53:36.4406485Z   <unknown>:0: note: see current operation: %1013 = "math.ctlz"(%1003) : (tensor<8x8x8xi64>) -> tensor<8x8x8xi64>

which should be solved once EnzymeAD/Enzyme-JAX#2578 hits a new Reactant_jll

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm more concerned as to who emits ctlz. I'm assuming its not coming from you guys, which implies it may be the defn of some libdevice implementation for sinpi or something. Obviously we shouldn't import that version but sinpi [or whatever is the actual cause] directly

@Pangoraw Pangoraw Jun 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Pinpointed it to fma_emulated used in ConvertingDivision{Float32}. It itself calls _exponent_finite_nonzero . Maybe we it could makes sense to use NativeDivision for Reactant since it does not have fma?

Comment thread .github/workflows/CI.yml Outdated
@dkytezab

Copy link
Copy Markdown
Collaborator Author

This looks good to me? I think we should also add a timestepping loop but that can be a subsequent PR imo, anticipating other correctness issues

@giordano giordano closed this Jun 25, 2026
@giordano giordano reopened this Jun 25, 2026
Comment thread test/reactant/correctness.jl Outdated
Comment thread test/reactant/correctness.jl Outdated
Co-authored-by: Mosè Giordano <765740+giordano@users.noreply.github.com>
@giordano

Copy link
Copy Markdown
Member

This looks good to me?

Yes! But we need to wait for EnzymeAD/Reactant.jl#2984 making it into a proper release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

reactant ☣ towards a differentiable earth testing 🧪

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Reactant correctness check for AtmosphereModel timestepping

4 participants