Make the NEMO total-energy under-relaxation check reachable - #2884
Merged
bigfooted merged 8 commits intoSep 16, 2026
Merged
Conversation
In CNEMOEulerSolver::ComputeUnderRelaxationFactor the total-energy check (iVar == nVar-2) was nested inside the species block (iVar < nSpecies). Total energy sits at nSpecies+nDim, so the check could never run and the energy update was never under-relaxed. Close the species block first and evaluate the energy check on its own; no new logic is added. This changes the implicit update wherever the energy bound is active, so the affected NEMO regression vectors are rebaselined (serial: invwedge, visc_cone; parallel: invwedge_a, invwedge_msw, invwedge_roe, invwedge_lax, invwedge_ss_inlet, visc_cone, super_cat, ion_gy). The aarch64 vectors are left for a maintainer. A unit test builds a NEMO Euler solver on the unit box and checks that an over-large energy update is limited (it stays unlimited on the previous code) alongside a species control. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Update the branch-specific serial and two-rank aarch64 expectations from GitHub Actions run 33998938091 at exact pre-update head a495681. Artifact digest: sha256:4c9d814dd9a373592fce4c9f33f74e565e26016ee855b2df8660bbcc99e4d560. No x86 expectation or solver source changes.
Contributor
|
you don't have to write almost 100 lines to explain basically the movement of a closing brace :-) |
pcarruscag
reviewed
Sep 9, 2026
Extract the per-point limiter into a shared function and test it with manufactured inputs. Validation: 9 limiter assertions and 33 supported native unit cases pass. Restoring the original brace error fails 4 limiter assertions. The data-driven fluid-model test is excluded because MLPCpp is disabled in this local build.
pcarruscag
approved these changes
Sep 15, 2026
pcarruscag
left a comment
pcarruscag
left a comment
Member
There was a problem hiding this comment.
LGTM, some regressions are failing, maybe from floating point changes due to taking out the function, please update the gold values when you can.
…_energy_underrelaxation
Refresh eight reference arrays from independently repeated Linux serial and two-rank MPI runs on x86 and ARM. All x86 selected outputs match upstream CI. Keep the reviewed limiter helper and tests, existing tolerances, ion iteration 99 and restart protection.
Remove the 37 existing E221 spacing issues reported by CodeFactor. Both complete Python ASTs and every numerical reference remain identical.
Author
|
Thanks Pedro. I updated the gold values after repeating the affected cases on Linux x86 and ARM. The repeated runs agree, and the tolerances are unchanged. The helper and its direct tests are still in place. The full CI run is underway. |
…_energy_underrelaxation
The serial and MPI cone rows repeat exactly on Linux x86 and ARM. Update the four cone arrays for the combined energy and distance fixes. All four full normal unit runs passed, and all 52 recorded rows accept the updated references with unchanged tolerances.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The total energy check was nested inside the species block, so it could never execute. This makes the energy check independent and applies the tighter species or total energy limit.
The loop logic is now the small
ComputeUnderRelaxationFactorhelper requested in review. Its direct test with manufactured data covers small and excessive energy updates, opposing species updates, which limit wins, and cancellation for a tiny factor.Validation after the latest develop merge
Updated with develop
8118ee0f507b5fb0fd70b819e6f1351c1b3c6d28after #2883 merged. The approved helper and its direct tests are unchanged.The combined source validation tested merge commit
7e89cb6be1a869a61f9425bd069dc1ac6aa3d699on Linux x86 and ARM with separate serial and MPI builds. All 52 solver runs completed, and every repeated row agreed within its architecture and build. All four full normal unit suites passed. Both MPI builds included Mutation++ support.That validation run failed its comparisons against the old cone references. Commit
53b8b75affdf3b1c29ff56691fc2333dd32cbd88updates four cone arrays from the repeated results, with three residual entries changed in each. All 52 recorded rows pass the updated references under the original tolerances. Other references, configurations and restart inputs are unchanged.Full upstream CI on
53b8b75affdf3b1c29ff56691fc2333dd32cbd88passed: all 31 regression workflow jobs, including unit tests and both sanitizer suites. Code Style, CodeQL, labels and CodeFactor also passed.PR Checklist
pre-commit run --allto format old commits.