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.
This branch has not been deployed
4 participants
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.