Repository navigation
Address leftover Copilot review comments from merged PRs - #1959
Open
sbryngelson wants to merge 6 commits into
Open
sbryngelson wants to merge 6 commits into
sbryngelson wants to merge 6 commits into
Conversation
…text The faulted-node sacct pipeline ran under set -e with no || true, so a failing sacct ended the script on the node-recovery path. Match the other sacct call. Also say why preflight matches "Illegal instruction" output.
…builds Only the offload link goes through clang-linker-wrapper, so CPU builds with LLVMFlang keep linker-reported dependencies.
- Build the grcbc_in required inflow fields as one list. - Default optional EOS coefficients only when unset (is None), not when falsy. - test_thermochem: split the Cray flags dict; key the cached compiler probe on $FC.
3D_ibm_neighborhood_radius takes the rank topology from TOPOLOGY (default 16,2,2) and uses the thinnest rank in any direction. 2D_ibm_thin_plate_force drops the unused LEVELS dict.
f(t0) is frac0 only to within ~0.25%·(1 - frac0). The index.html change is whitespace only.
- time_final / io_time_final comments name the max over ranks. - Drop the measured drift figure from the restart-clock comment. - real(t_step + 1, wp)*dt in place of implicit integer conversion. - Inflow ramp comment: f(t0) and f(t0 + tau) are approximate.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1959 +/- ##
=======================================
Coverage 61.79% 61.79%
=======================================
Files 86 86
Lines 22773 22773
Branches 3353 3353
=======================================
Hits 14073 14073
Misses 6211 6211
Partials 2489 2489 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This branch has not been deployed
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
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.
Follow-ups to Copilot review comments left open on PRs that are already merged. Each item was checked against current master before it was changed. Results do not change: the Fortran edits are comments plus one explicit
real(wp)cast, andindex.htmlis whitespace only (git diff -wis empty).Made with Claude Code.
Bug
submit-slurm-job.sh(thread): thesacctfallback that finds the faulted node had no|| true. Underset -euo pipefail, a failingsacctended the script without a message on the node-recovery path. It now ends in|| true, like the othersacctcall (~L198).Minor
case.mdnow says "≈ f0", and the comment inm_derived_types.fppsays the values at t0 and t0 + τ are approximate.case_validator.py(thread): thegrcbc_ininflow fields are built as onerequiredlist, andmissingis computed from it in a single pass. The fields and the error message order are unchanged.examples/3D_ibm_neighborhood_radius/case.py(thread):SUMMARYno longer hard-codes 16 x 2 x 2. It readsTOPOLOGY(default16,2,2), computes the rank count and the extent in each direction, and uses the thinnest rank in any direction. With the default topology the output is byte-identical.examples/2D_ibm_thin_plate_force/case.py(thread): removed the unusedLEVELS. The comment above it referred to "L4", so it now says NCELL=16.case_validator.py(thread):value or 0.0is replaced by an explicitis Nonecheck.preflight.sh(thread): the"Illegal instruction"text match is kept on purpose. A comment now explains why: some launchers do not pass exit code 132 on.CMakeLists.txt(thread 1, thread 2):CMAKE_LINK_DEPENDS_USE_LINKER OFFnow applies only when the compiler isLLVMFlang AND MFC_OpenMP. That combination is the amdflang offload build (MFCTargets.cmake adds--offload-archunder it), so GPU builds behave as before and LLVMFlang CPU builds keep the linker's dependency tracking.docs/index.html(thread 1, thread 2): all 40simsrows are now column-aligned to one width per field. The old rows did not share columns, and the new names are too long to fit the old ones.test_thermochem.py(thread 1, thread 2): the one-line"Cray"flags dict is now split across lines. The cached compiler probe moved to_probe_compiler(fc), so the cache is keyed on$FCand a changed$FCis probed again.m_start_up.fpp(thread): the comments now saytime_finalis the max over ranks of each rank's fastest RK stage, andio_time_finalis the max over ranks of each rank's means_save_datatime.m_start_up.fpp/m_time_steppers.fpp(thread 1, thread 2): removed the "1290 ulps by step 27000" figure and kept the reason. The fixed-dt clock now usesreal(t_step + 1, wp)*dtand(real(t_step, wp) + 0.5_wp)*dt, in boths_perform_time_stepand the IB stage time, so the two stay the same expression. The integer-to-real conversion is exact either way, so the values are bitwise unchanged.Not changed
"240-512 GCDs". A later commit on that PR (f6f6ce1, "Update accelerators for Seagull simulation") deliberately set it to"512 GCDs", and the merged PR description matches. There is no case file in the repo to check it against. Going back to the range would undo that deliberate edit, so the entry stays as it is.Verification
./mfc.sh format: clean./mfc.sh precheck(formatting, spelling, toolchain and source lint, doc refs, param docs, example validation): all 7 checks pass./mfc.sh lint(ruff + toolchain unit tests): 794 passed, 2 skippedpytest toolchain/mfc/test_thermochem.pywith CCEftn: 16 passed, 1 skippedbash -nonpreflight.shandsubmit-slurm-job.sh: OK./mfc.sh validateon both edited examples: pass. Agrcbc_incase with fields removed still reports them in the same order../mfc.sh build -j 8(Frontier, CPU): OK./mfc.sh test -j 8 -% 10on a Frontier CPU node: 72 passed, 0 failed./mfc.sh test -j 8 -o IBM(includes the fixed-dt prescribed-kinematics and moving-IB goldens): 61 passed, 0 failed./mfc.sh test -o <6 grcbc UUIDs>(1D/2D/3D grcbc x/y/z): 6 passed, 0 failedAcknowledgement
PR template credit: junegunn