Repository navigation
Conversation
…aries The residual of the periodic match is added as Q*R, and the solution of the match is Q*U, so the block added to the diagonal must be Q*J*Q^T. Only the rows were rotated. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013UkNcoCEH8nFNrHWzhJCar
The coarse levels summed the momentum residuals, gradients and Jacobian blocks of a rotational periodic pair without rotating them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013UkNcoCEH8nFNrHWzhJCar
…stencil The limiters of the velocity components were rotated like a vector when taking the minimum over a periodic pair, and the min and max velocity vectors were rotated instead of the velocity of each neighbour. Now each side sends, in the frame of its match, the min/max of the rotated velocities and of the rotated reconstruction increments, and the limiter is computed once. Nothing changes without rotation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013UkNcoCEH8nFNrHWzhJCar
Test casesFiles (configurations, histories, meshes, statistics, scripts) for each case: periodicBoundaries/rotation. 2 MPI ranks, "develop" = All 2D runs use
1. Jacobian block: New test 2. Multigrid: the same case at CFL 20 with
New test 3. Limiters: one iteration from the same field on the sector and on the full annulus (8 copies of the sector, no periodic boundary),
Largest difference of the velocity limiters to the annulus, on the periodic points:
In 3D ( The original case (CFL 100,
The direct-flow translation checks Existing testsReference values that change:
The three turbomachinery cases have a rotational periodic boundary; a build with only the Jacobian fix gives the same new values. Build and adjoint checksStandalone OpenMP/MPI release build at warning level 3, with mixed precision. The three MPI periodic2D tests meet their references within the regression tolerance; the existing hybrid periodic2D, Jones and multi_interface runs succeed, and their references are updated above. No new warnings from the periodic changes; pre-commit checks pass. Earlier checks of the combined periodic branch passed the standard unit suite (44 cases / 74913 assertions) and reverse-AD units (4 cases / 29 assertions). The full OpenMP unit driver aborts in the data-driven fluid test when the optional MLPCpp support is disabled, so it is not reported as a passing suite. The rotational periodic2D adjoint smoke runs exit successfully, but the default linear-solver settings do not converge on either develop or the branch. Tighter ILU settings improve both; these are not a complete sensitivity validation. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
joshkellyjak
left a comment
joshkellyjak
left a comment
There was a problem hiding this comment.
Can you test this implementation on one of the turbomachinery testcases and add MG to their regression test? This seems like the area where this fix will have the most benefit
|
@joshkellyjak, the Aachen restart regression now uses The 10,000-iteration cluster comparison and MG1/2/3 study are complete. The PR’s final energy residuals are about 2.8 orders lower than develop; MG2/3 reduce late oscillations. The runs still show residual floors, efficiency drift and mass mismatch, so they are not fully converged. Full study: configs, inputs, histories, plots and reproduction scripts. Separate wall-function work is underway on #2955: keeping model wall temperature local removes the saved state inconsistency; cache refresh and inactive SA handling are being validated. The wall-solver experiment needs further investigation because it increases residual oscillations. Latest CI hit Eigen download errors; the retry is running. |
| /*! \return Whether any of the three supplied rotation angles is nonzero. */ | ||
| template <class Scalar> | ||
| inline bool HasRotation(const Scalar* angles) { | ||
| return angles[0] != 0.0 || angles[1] != 0.0 || angles[2] != 0.0; |
There was a problem hiding this comment.
you shouldn't do direct comparisons on floats, use tolerances instead:
abs(angles[0]) < EPS
There was a problem hiding this comment.
In this check, the angles come directly from the configuration: configured zero remains exactly zero after degree-to-radian conversion and donor negation. Keeping the exact comparison retains every nonzero rotation, including very small angles. I documented this in 12d95ac and added tests for signed zero and tiny positive/negative angles on all three axes. The focused tests pass.
joshkellyjak
left a comment
joshkellyjak
left a comment
There was a problem hiding this comment.
It is worth noting here that this implementation maybe gets closer to allowing periodic rotational boundaries in NEMO. However, in NEMO the index for velocity is not 1 as used in CSolver.cpp::461, it instead nSpecies+2. The way to fix this is to index these using an index struct, but given that there may be more issues related to periodics in NEMO and it is probably beyond the scope of this PR, I will instead leave this here and hopefully it is useful for some future NEMO developments.
|
@joshkellyjak Thanks, agreed. NEMO periodic communication is currently guarded, so this PR does not enable NEMO support. That follow-up will need solver-specific primitive and conservative indices throughout the periodic paths. |
|
For NEMO we could maybe first look at how to integrate it more, so a refactoring of the code to inherit more of the base solver. NEMO duplicates a lot of things. |
|
@bigfooted, I am sure we can improve the NEMO coupling. I can look at that after we finalize the current work, if it is not urgent. |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core periodic implicit/limiter communication and rotation logic across multigrid levels with broad solver-wide impact that warrants final human verification.
1 open finding
What changed in this PR
This PR addresses several correctness issues in SU2’s rotational periodic boundary handling that affected implicit Jacobian assembly, multigrid convergence, and slope limiter behavior, and adds regression/unit tests to lock in the fixes.
Changes:
- Fix implicit periodic Jacobian rotation to apply the rotation consistently on both sides (rows and columns) for rotational periodic matches.
- Enable periodic rotation handling across all multigrid levels and update periodic limiter communications to correctly handle rotational frames.
- Add new regression tests for rotational periodic cases (including multigrid) and a new unit test file for periodic limiter/projection helpers; refresh affected reference values.
| File | Description |
|---|---|
SU2_CFD/src/solvers/CSolver.cpp |
Updates periodic comm packing, Jacobian rotation handling, and adds rotational-periodic limiter projection support. |
SU2_CFD/include/solvers/CSolver.hpp |
Adds storage/indexing for periodic limiter projection data used with rotational periodicity. |
SU2_CFD/include/solvers/CFVMFlowSolverBase.inl |
Enables periodic rotation handling on all multigrid levels. |
SU2_CFD/include/limiters/computeLimiters_impl.hpp |
Incorporates rotational periodic “projection increment bounds” into limiter computation and aligns increment computation with MUSCL scaling helper. |
SU2_CFD/include/limiters/CLimiterDetails.hpp |
Adds a shared helper for MUSCL reconstruction increments. |
Common/include/toolboxes/geometry_toolbox.hpp |
Adds HasRotation and RotateBox helpers for robust component-bound rotation. |
UnitTests/SU2_CFD/periodic_limiters.cpp |
Adds unit tests for rotation detection, rotated bound enclosure, MUSCL increment scaling, and periodic projection storage behavior. |
UnitTests/meson.build |
Registers the new unit test file in the unit test build. |
TestCases/navierstokes/periodic2D/no_limiter.cfg |
Adds a new rotational periodic regression configuration (no limiter) targeting Jacobian periodic behavior. |
TestCases/navierstokes/periodic2D/multigrid.cfg |
Adds a rotational periodic regression configuration with multigrid enabled. |
TestCases/turbomachinery/Aachen_turbine/aachen_3D_MP_restart.cfg |
Sets MGLEVEL= 1 explicitly for the Aachen restart testcase. |
TestCases/serial_regression.py |
Updates reference values for affected rotational periodic/restart cases. |
TestCases/parallel_regression.py |
Adds new rotational periodic regression tests and refreshes affected reference values. |
TestCases/hybrid_regression.py |
Updates reference values for the hybrid rotational periodic case and related restarts. |
TestCases/parallel_regression_AD.py |
Refreshes reference values for an affected discrete-adjoint regression. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| case PERIODIC_LIM_PRIM_1: | ||
| COUNT_PER_POINT = nPrimVarGrad*2; | ||
| /*--- Min and max of the solution, min and max of the reconstruction increments. ---*/ | ||
| COUNT_PER_POINT = nPrimVarGrad*4; | ||
| MPI_TYPE = COMM_TYPE::DOUBLE; | ||
| ICOUNT = nPrimVarGrad; | ||
| break; |
Proposed Changes
This is one of seven related periodic-boundary bug-fix PRs.
The other bug-fix PRs are drafts with their remaining validation and implementation limits documented. Their grouping does not imply a linear merge order; #2963 depends on this PR.
The fully coupled implicit operator proposed in #2967 is a separate experimental enhancement. It has been closed and deferred because no overall benefit has been established and convergence/validation issues remain. This PR retains the existing implicit approximation.
Three fixes for rotational periodic boundaries, one commit each. The test cases are in the comment below.
New tests
periodic2d_no_limiterandperiodic2d_multigrid;periodic2dis now also in the MPI regression. Their x86 reference values pass in CI.Existing reference values that change:
aachen_turbine_restart,jones_turbocharger_restartandmulti_interface(rotational periodic, the Jacobian block), and the hybridperiodic2d(the Jacobian block and the limiters). The x86 periodic tests and the other changed cases pass in CI; the Jones restart residual references were refreshed from the serial, MPI and hybrid CI logs. The aarch64 references still need the CI runs.No conflict with #2945 and #2959. #2956 also changes the reference values of
jones_turbocharger_restart; whichever is merged second takes them from the CI.Related Work
periodic2Dcase used here.PR Checklist
pre-commit run --allto format old commits.