Skip to content

Address leftover Copilot review comments from merged PRs - #1959

Open
sbryngelson wants to merge 6 commits into
MFlowCode:masterfrom
sbryngelson:ai-review-followups
Open

sbryngelson wants to merge 6 commits into
MFlowCode:masterfrom
sbryngelson:ai-review-followups

Conversation

@sbryngelson

@sbryngelson sbryngelson commented Oct 8, 2026 •

Copy link
Copy Markdown
Member

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, and index.html is whitespace only (git diff -w is empty).

Made with Claude Code.

Bug

Minor

Not changed

  • docs: add five flapping-flight and jet runs to the showcase #1912, "512 GCDs" (thread): the PR first had "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 skipped
  • pytest toolchain/mfc/test_thermochem.py with CCE ftn: 16 passed, 1 skipped
  • bash -n on preflight.sh and submit-slurm-job.sh: OK
  • ./mfc.sh validate on both edited examples: pass. A grcbc_in case with fields removed still reports them in the same order.
  • ./mfc.sh build -j 8 (Frontier, CPU): OK
  • ./mfc.sh test -j 8 -% 10 on 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 failed

Acknowledgement

  • I confirm this PR meets the above expectations and reflects my own understanding and real-world context.

PR template credit: junegunn

…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.
Copilot AI balanced review requested due to automatic review settings October 8, 2026 18:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@codecov

codecov Bot commented Oct 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 61.79%. Comparing base (27cae2c) to head (65134e3).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/simulation/m_time_steppers.fpp 50.00% 0 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants