Skip to content

Use Rosenbrock solver for photoionization#2000

Merged
chongchonghe merged 8 commits into
developmentfrom
chong/photo/use-rosenbroke
Jun 26, 2026
Merged

Use Rosenbrock solver for photoionization#2000
chongchonghe merged 8 commits into
developmentfrom
chong/photo/use-rosenbroke

Conversation

@chongchonghe

@chongchonghe chongchonghe commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Description

Switch from VODE to Rosenbrock solver with rosenbrock_tableau = 3 (3-stage ROS2S tableau). Update all photoionization test problem to use this new solver.

Performance results from Ben's tests

From @BenWibking 's message on Slack:

Ok, I've re-tested the performance of all the integrators following @chongchong He’s tolerance changes. all of them are now fast, but Rosenbrock with ROS2S is fastest, about 20% faster than VODE:

  │ Method  │ FoM us/zone │ Mupdate/s │ TP wall s │ Chem s │ Chem % │ Rad-noODE % │ Hydro % │ Hydro-only x │ RadSub/HydroStep │ Subcyc │
  ├─────────┼─────────────┼───────────┼───────────┼────────┼────────┼─────────────┼─────────┼──────────────┼──────────────────┼────────┤
  │ ROS2S   │      0.0877 │     11.40 │     19.24 │   6.47 │  33.65 │       42.80 │   14.78 │         6.77 │             0.52 │   9.99 │
  │ Rodas3P │      0.0895 │     11.17 │     19.30 │   7.07 │  36.62 │       41.82 │   14.43 │         6.93 │             0.54 │   9.99 │
  │ Rodas4P │      0.0957 │     10.45 │     20.73 │   7.95 │  38.35 │       40.64 │   13.51 │         7.40 │             0.58 │   9.99 │
  │ VODE    │      0.1089 │      9.18 │     23.20 │  10.67 │  45.98 │       36.15 │   12.05 │         8.30 │             0.68 │   9.99 │
  │ Rodas5P │      0.1214 │      8.24 │     25.93 │  13.09 │  50.47 │       32.48 │   11.08 │         9.03 │             0.75 │   9.99 │
  └─────────┴─────────────┴───────────┴───────────┴────────┴────────┴─────────────┴─────────┴──────────────┴──────────────────┴────────┘

With ROS2S, the cost of one radiation subcycle is ~0.5 times the cost of one hydro update.

and the ODE solve only takes about 33% of the total walltime for the DTypeFront problem

However, this is on an H200 GPU, so it would be good to also do this comparison for MI250X.

Related issues

Not ready yet.
Need a PR to update Microphysics module to add radiation to Rosenbrock.

Checklist

Before this pull request can be reviewed, all of these tasks should be completed. Denote completed tasks with an x inside the square brackets [ ] in the Markdown source below:

  • I have added a description (see above).
  • I have added a link to any related issues (if applicable; see above).
  • I have read the Contributing Guide.
  • I have added tests for any new physics that this PR adds to the code.
  • (For quokka-astro org members) I have manually triggered the GPU tests with the magic comment /azp run.

@chongchonghe

Copy link
Copy Markdown
Contributor Author

/azp run rocm-quick

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the configuration files to set the integrator.rosenbrock_tableau parameter to 3 and modifies the CMakeLists.txt files for the DTypeFront and StromgrenSphere problems to include 'Rosenbrock' in the microphysics compilation setup. I have no feedback to provide as there were no review comments.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@chongchonghe chongchonghe marked this pull request as ready for review June 25, 2026 09:22
@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. enhancement New feature or request labels Jun 25, 2026
@chongchonghe chongchonghe enabled auto-merge June 25, 2026 09:23
@chongchonghe chongchonghe requested a review from James471 June 25, 2026 09:23

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a668f7d901

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/problems/DTypeFront/CMakeLists.txt
Comment thread inputs/StromgrenRSLA.toml
Comment thread src/problems/StromgrenSphere/CMakeLists.txt
@chongchonghe chongchonghe marked this pull request as draft June 25, 2026 10:22
auto-merge was automatically disabled June 25, 2026 10:22

Pull request was converted to draft

@James471

Copy link
Copy Markdown
Contributor

Rosenbrock needs to be selected in StromgrenSphereRSLA as well. And if we want, we can also do so in OneZonePhotoionization. ROS2S needs to be selected in StromgrenNoRecombination.toml. And potentially, in the two OneZone input files as well. ROS2S is slow for the one zone problems, so we may choose not to do it.

@chongchonghe

Copy link
Copy Markdown
Contributor Author

/azp run rocm-quick

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@BenWibking BenWibking changed the title Use Rosenbroke solver for photoionization Use Rosenbrock solver for photoionization Jun 25, 2026
@chongchonghe

Copy link
Copy Markdown
Contributor Author

The question is whether or not to move from VODE to Rosenbrock entirely, or we do want to have some problems testing the VODE solver. @James471 @BenWibking

@BenWibking

BenWibking commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

The question is whether or not to move from VODE to Rosenbrock entirely, or we do want to have some problems testing the VODE solver. @James471 @BenWibking

Since Rosenbrock is faster for the DTypeFront problem, and we plan to use it in production, I don't think it's necessary to keep any problems that use VODE.

I think we only need to test VODE if it were going to be used in a production simulation. Howver, VODE only appears to be the fastest method when the tolerances are extremely small (close to machine precision) and the species are far from chemical equilibrium. I don't think we have any applications that require this.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@chongchonghe

Copy link
Copy Markdown
Contributor Author

/azp run rocm-quick

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@chongchonghe chongchonghe marked this pull request as ready for review June 26, 2026 04:22
@dosubot dosubot Bot added the submodules Pull requests that update Submodules code label Jun 26, 2026
@chongchonghe

Copy link
Copy Markdown
Contributor Author

/azp run rocm-quick

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@chongchonghe

Copy link
Copy Markdown
Contributor Author

@BenWibking @James471 Ready for review again.

@chongchonghe chongchonghe enabled auto-merge June 26, 2026 04:52
@chongchonghe

Copy link
Copy Markdown
Contributor Author

/azp run quick

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

James471
James471 previously approved these changes Jun 26, 2026
@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Jun 26, 2026
@chongchonghe

Copy link
Copy Markdown
Contributor Author

@BenWibking @markkrumholz Can you approve? This is ready to merge.

@BenWibking

Copy link
Copy Markdown
Collaborator

/claude-review

@claude

claude Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @BenWibking's task in 1m 40s —— View job


Code Review — Use Rosenbrock solver for photoionization

I reviewed the diff against origin/development. This is a clean, well-scoped configuration change. The earlier Codex/gemini concerns have largely been addressed by the most recent commits.

Tasks

  • Read the full diff and changed files
  • Cross-check every photoionization test target/input is covered
  • Re-check the earlier reviewer concerns
  • Post review

Completeness check ✅

I traced each add_test to its .toml and confirmed the solver switch is applied consistently on both the build side (CMake "Rosenbrock" argument) and the runtime side (integrator.rosenbrock_tableau = 3):

Test target Input(s) rosenbrock_tableau=3 CMake "Rosenbrock"
DTypeFront DTypeFront.toml
StromgrenSphere* StromgrenNoRecombination.toml, StromgrenTempIndependentRecombination.toml ✅ / ✅
StromgrenSphereRSLA StromgrenRSLA.toml
OneZonePhotoionization* OneZonePhotoionization.toml, OneZonePhotoionizationWithEnergy.toml ✅ / ✅

This resolves the three earlier Codex comments — the no-recombination input now sets the tableau (StromgrenNoRecombination.toml:36), and the RSLA target now passes "Rosenbrock" to the helper (StromgrenSphereRSLA/CMakeLists.txt:8). Nothing falls back to the default Microphysics tableau anymore. 👍

Main thing to confirm before merge ⚠️

.gitmodules now points the Microphysics submodule at a personal fork on a feature branch:

-	url = https://github.com/AMReX-Astro/Microphysics.git
-	branch = development
+	url = https://github.com/chongchonghe/Microphysics-fork.git
+	branch = photochemistry

The Codex P1 comment (whether setup_target_for_microphysics_compilation(..., "Rosenbrock") actually puts Rosenbrock — not VODE — on the photoionization include path) hinges entirely on this fork; @James471 confirmed it's fixed by the fork. I cannot verify the helper's signature/behavior here because the submodule isn't checked out in this environment, so that part rests on the GPU/rocm-quick CI actually exercising ROS2S.

Two recommendations:

  1. Do not merge development with the submodule URL pointing at chongchonghe/Microphysics-fork. The PR body itself notes "Need a PR to update Microphysics module to add radiation to Rosenbrock." Quokka's development should track upstream AMReX-Astro/Microphysics — once that upstream PR lands, repoint the submodule (URL and branch) back to AMReX-Astro/Microphysics and bump the commit. Merging against a personal fork branch is fragile (force-pushes/branch deletion break the build for everyone). This is flagged "Not ready yet" in Related issues, so this likely just needs a final follow-up before merge.
  2. Sanity-check the CI did the right thing: since the include-path fix lives in the submodule, the only real guard that these tests run ROS2S rather than silently building VODE is the GPU pipeline. Worth confirming rocm-quick/quick passed and that the runs reflect the ~20% speedup, not just that they compiled.

Minor / non-blocking

  • inputs/OneZonePhotoionization.toml has no trailing newline (\ No newline at end of file). Pre-existing, but easy to fix while touching the file. Fix this →
  • Per @James471's note, ROS2S is reportedly slow for the one-zone problems (tight rtol = 1e-14). Applying it there is fine as a solver-coverage test, but if CI time matters you may prefer to leave the one-zone problems on VODE. A design call, not a defect.

Summary

Logic and completeness look good — every photoionization test is now consistently switched to ROS2S at both build and runtime. No physics, GPU-lambda, or style issues in this diff (it's config/CMake only). The only blocker is the submodule pointing at a personal fork; resolve that (or confirm it's an intentional pre-upstream-merge state) before merging to development.
chong/photo/use-rosenbroke

@BenWibking BenWibking left a comment

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.

Everything looks good, except for the submodule URL issue.

Comment thread .gitmodules Outdated
@dosubot dosubot Bot removed the lgtm This PR has been approved by a maintainer label Jun 26, 2026
@chongchonghe

chongchonghe commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

@BenWibking I've reverted the submodule url. Ready again.

@BenWibking BenWibking left a comment

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.

LGTM

@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Jun 26, 2026
@chongchonghe chongchonghe added this pull request to the merge queue Jun 26, 2026
Merged via the queue into development with commit fa645a8 Jun 26, 2026
53 checks passed
@BenWibking BenWibking deleted the chong/photo/use-rosenbroke branch June 26, 2026 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request lgtm This PR has been approved by a maintainer size:S This PR changes 10-29 lines, ignoring generated files. submodules Pull requests that update Submodules code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants