Skip to content

Refactor Reactant microphysics tests + test P3 compilation - #1017

Merged
dkytezab merged 11 commits into
mainfrom
reactantmicrophysics
Sep 29, 2026
Merged

dkytezab merged 11 commits into
mainfrom
reactantmicrophysics

Conversation

@dkytezab

Copy link
Copy Markdown
Collaborator

No description provided.

Comment thread src/Microphysics/PredictedParticleProperties/ice_aggregation_rates.jl Outdated
Comment thread src/Microphysics/PredictedParticleProperties/process_rate_helpers.jl Outdated
@codecov

codecov Bot commented Sep 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@giordano giordano added the reactant ☣ towards a differentiable earth label Sep 22, 2026
@giordano

Copy link
Copy Markdown
Member

I reverted the changes to the source code, but this is failing to raise a function (was failing before also before my change), probably needs more work on the Reactant side?

Comment on lines +360 to 365
@inline function clamp_negative_numbers!(i, j, k, fields::Tuple{F, Vararg}) where {F}
f = fields[1]
@inbounds f[i, j, k] = max(0, f[i, j, k])
clamp_negative_numbers!(i, j, k, Base.tail(fields))
return nothing
end

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 think the raising issue is with this function that iterates over a tuple of offset arrays. i rewrote it in a similar manner to zero_orphaned_numbers! so it becomes unrolled.

@giordano giordano Sep 27, 2026 •

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 guess this change is the cause of the huge performance drop in Reactant benchmarks. Edit: maybe not, after all.

@numterra-bot numterra-bot Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Breeze.jl Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.10.

Benchmark suite Current: 028de58 Previous: 82d9622 Ratio
ModelTendency; Grid: 256x256x128/Advection: WENO7/NVIDIA L4/F32 reactant raise=true 131183253.64778502 points/s 693851521.5735954 points/s 5.29
ModelTendency; Grid: 256x256x128/Advection: WENO9/NVIDIA L4/F32 reactant raise=true 18996418.14392214 points/s 52738861.65621344 points/s 2.78
ScalarTendency; Grid: 256x256x128/Advection: WENO5/NVIDIA L4/BF16 reactant raise=true 10836174802.681719 points/s 11987613125.97085 points/s 1.11
ScalarTendency; Grid: 256x256x128/Advection: WENO7/NVIDIA L4/BF16 reactant raise=true 5972524675.461524 points/s 6953248841.623632 points/s 1.16
ScalarTendency; Grid: 256x256x128/Advection: WENO9/NVIDIA L4/F32 reactant raise=true 94645195.31787553 points/s 273872334.52941924 points/s 2.89

This comment was automatically generated by workflow using github-action-benchmark.

@giordano

Copy link
Copy Markdown
Member

Oh wow, that's a massive slowdown

@giordano

Copy link
Copy Markdown
Member

This is good to go from my point of view, I'll leave it to @Pangoraw and @dkytezab to decide what to do with the performance regression (merge now the tests and deal with the regression later, or wait to resolve the regression before pushing this forward).

@giordano

giordano commented Sep 27, 2026 •

Copy link
Copy Markdown
Member

I should note that at the same time ScalarTendency; Grid: 256x256x128/Advection: WENO9/NVIDIA L4/BF16 reactant raise=true got a 90% speedup, recovering from the drop in #923 (but WENO9 with bfloat16 is probably an unusual combination in practice). No other benchmarks got significant improvements.

@giordano

Copy link
Copy Markdown
Member

Uh, based on #1036 (review) the regressions may come from Reactant v0.2.289 and not changes in this PR, so this should be good to go.

@Pangoraw

Copy link
Copy Markdown
Collaborator

I'll investigate the regression, I don't think this was caused by the code change in this PR.

@giordano

Copy link
Copy Markdown
Member

No, it's happening elsewhere as well, so it's probably new Reactant version. This is ready from my point of view

@dkytezab
dkytezab merged commit 4c0f6a3 into main Sep 29, 2026
24 checks passed
@dkytezab
dkytezab deleted the reactantmicrophysics branch September 29, 2026 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

reactant ☣ towards a differentiable earth

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants