Skip to content

Deterministic MLMG#4855

Merged
WeiqunZhang merged 14 commits intoAMReX-Codes:developmentfrom
chongchonghe:chong/produce
Dec 20, 2025
Merged

Deterministic MLMG#4855
WeiqunZhang merged 14 commits intoAMReX-Codes:developmentfrom
chongchonghe:chong/produce

Conversation

@chongchonghe
Copy link
Copy Markdown
Contributor

@chongchonghe chongchonghe commented Dec 16, 2025 •
edited
Loading

Summary

The MLMG solver is not reproducible because it uses non-deterministic atomic operations in reflux. This PR adds an option to make the MLMG solver deterministic:

amrex::LPInfo mg_info;
mg_info.setDeterministic(true); // Enable deterministic mode for bitwise reproducibility
// then pass it to either `amrex::OpenBCSolver` or `amrex::MLPoisson`
amrex::MLPoisson mlpoisson(Geom(0, finest_level), boxArray(0, finest_level), DistributionMap(0, finest_level), mg_info);

@BenWibking

Additional background

Checklist

The proposed changes:

  • fix a bug or incorrect behavior in AMReX
  • add new capabilities to AMReX
  • changes answers in the test suite to more than roundoff level
  • are likely to significantly affect the results of downstream AMReX users
  • include documentation in the code and/or rst files, if appropriate

@WeiqunZhang
Copy link
Copy Markdown
Member

WeiqunZhang commented Dec 16, 2025

One way to do this might be:

  1. Add an optional boolean argument to ParallelCopy and ParallelAdd. Use that to control the unpacking behavior in AMReX_PCI.H.
  2. Add a new boolean member of YAFluxRegister and use it in ParallelAdd.
  3. Add a new boolean member to LpInfo in AMReX_MLLinOp.H and use that for the YAFluxRegiseter's setting.

@chongchonghe chongchonghe marked this pull request as draft December 17, 2025 10:17
@chongchonghe chongchonghe marked this pull request as ready for review December 17, 2025 10:23
@chongchonghe
Copy link
Copy Markdown
Contributor Author

chongchonghe commented Dec 17, 2025 •
edited
Loading

This is ready for review. @WeiqunZhang

I tested it in Quokka, and it worked well.

@chongchonghe chongchonghe mentioned this pull request Dec 17, 2025
Merged
5 tasks
@BenWibking
Copy link
Copy Markdown
Contributor

BenWibking commented Dec 18, 2025

@chongchonghe I think there are clang-tidy warnings to fix:

../../../../Src/Base/AMReX_FabArrayCommI.H:414:58: error: parameter 'deterministic' is unused [misc-unused-parameters,-warnings-as-errors]
Suppressed 41842 warnings (41827 in non-user code, 15 NOLINT).
  414 |                                     bool                 deterministic)

WeiqunZhang
WeiqunZhang reviewed Dec 18, 2025
View reviewed changes
Src/Base/AMReX_FabArrayCommI.H
if constexpr (amrex::IsAddAssignable<value_type>::value) {
FB_local_add_gpu(TheFB, scomp, ncomp, deterministic);
} else {
amrex::Abort("SumBoundary requires operator+=");
Copy link
Copy Markdown
Member

@WeiqunZhang WeiqunZhang Dec 18, 2025

Choose a reason for hiding this comment

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

Could we revert the white space change here? It makes the indentation inconsistent.

Copy link
Copy Markdown
Contributor Author

@chongchonghe chongchonghe Dec 19, 2025

Choose a reason for hiding this comment

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

Done

WeiqunZhang
WeiqunZhang reviewed Dec 18, 2025
View reviewed changes
Tests/LinearSolvers/ABecLaplacian_C/MyTest.cpp Outdated
info.setAgglomeration(agglomeration);
info.setConsolidation(consolidation);
info.setMaxCoarseningLevel(max_coarsening_level);
info.setDeterministic(true);
Copy link
Copy Markdown
Member

@WeiqunZhang WeiqunZhang Dec 18, 2025

Choose a reason for hiding this comment

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

Could we make this a runtime parameter that defaults to false? We often use this test for performance testing. This might change the performance measurement

Copy link
Copy Markdown
Contributor Author

@chongchonghe chongchonghe Dec 19, 2025

Choose a reason for hiding this comment

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

I randomly picked a test problem to turn on setDeterministic. Can you recommend another problem to test this feature that does not affect your performance testing?

Copy link
Copy Markdown
Contributor Author

@chongchonghe chongchonghe Dec 19, 2025

Choose a reason for hiding this comment

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

Do we need to update a problem to test this new feature?

Copy link
Copy Markdown
Contributor Author

@chongchonghe chongchonghe Dec 19, 2025

Choose a reason for hiding this comment

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

@WeiqunZhang Do you mean to make this a runtime parameter of the test problem, or as a runtime parameter for the whole amrex library, like amrex.deterministic_reduction?

Copy link
Copy Markdown
Member

@WeiqunZhang WeiqunZhang Dec 19, 2025

Choose a reason for hiding this comment

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

I meant a parameter just in that test.

Copy link
Copy Markdown
Member

@WeiqunZhang WeiqunZhang Dec 19, 2025

Choose a reason for hiding this comment

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

Let's add a runtime parameter say deterministic to Tests/LinearSolver/ABecLaplacian_C with a default value of false and add deterministic=1 to inputs-rt-abcelap-com, which is part of our nightly regression tests.

@WeiqunZhang
Copy link
Copy Markdown
Member

WeiqunZhang commented Dec 18, 2025

/run-hpsf-gitlab-ci

@github-actions
Copy link
Copy Markdown

github-actions bot commented Dec 18, 2025

GitLab CI has started at https://gitlab.spack.io/amrex/amrex/-/pipelines/1356360.

@amrex-gitlab-ci-reporter
Copy link
Copy Markdown

amrex-gitlab-ci-reporter bot commented Dec 18, 2025

GitLab CI 1356360 finished with status: success. See details at https://gitlab.spack.io/amrex/amrex/-/pipelines/1356360.

@chongchonghe
Copy link
Copy Markdown
Contributor Author

chongchonghe commented Dec 20, 2025

@WeiqunZhang OK, I have added a runtime parameter to ABecLaplacian_C as you suggested. It's ready for review again.

@WeiqunZhang WeiqunZhang merged commit b647b95 into AMReX-Codes:development Dec 20, 2025
73 checks passed
github-merge-queue bot pushed a commit to quokka-astro/quokka that referenced this pull request Dec 21, 2025
@chongchonghe
Make gravity+AMR fully reproducible (#1524)
fa51f4e 
### Description
This PR makes gravity + AMR fully reproducible, and thus makes the whole
code (hydro + radiation + gravity + particles) fully reproducible.

Dependent on AMReX-Codes/amrex#4855, which is
now in the amrex development branch

### Related issues
Are there any GitHub issues that are fixed by this pull request? Add a
link to them here.

### 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](https://github.com/quokka-astro/quokka/blob/development/CONTRIBUTING.md).
- [ ] 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`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@WeiqunZhang WeiqunZhang WeiqunZhang approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@chongchonghe @WeiqunZhang @BenWibking