Skip to content

Label time_data.dat and io_time_data.dat columns by what they measure - #1949

Merged
sbryngelson merged 1 commit into
MFlowCode:masterfrom
sbryngelson:fix-time-data-labels
Oct 8, 2026
Merged

sbryngelson merged 1 commit into
MFlowCode:masterfrom
sbryngelson:fix-time-data-labels

Conversation

@sbryngelson

@sbryngelson sbryngelson commented Oct 7, 2026 •

Copy link
Copy Markdown
Member

Description

The time_data.dat header calls its second column s/step, but the value is the fastest single RK stage, i.e. one RHS evaluation. For RK3 that is about a third of a step, so anyone reading it as a per-step time underestimates cost by about 3x. The io_time_data.dat column is also labelled s/step, but it holds the mean time of one s_save_data call.

Root cause. Since #1650, s_tvd_rk sets time_avg to the minimum wall-clock time over individual RK stages, which is the right input for the grind time (ns/gp/eq/rhs). The header written in s_save_performance_metrics was never updated.

Fix. The headers become s/rhs and s/save, and each gets a one-line comment. Columns, values and formats are unchanged. The only parser in the tree (helpers.mako, which reads the last column of the last line) is unaffected. An existing time_data.dat keeps its old header, because rows are appended.

Verification

I ran a 1-rank run of a small 2D case on a Frontier CPU compute node (GNU 12.3):

time_data.dat:
     Ranks          s/rhs   ns/gp/eq/rhs
         1     0.00137424   143.82386185
io_time_data.dat:
     Ranks         s/save
         1     0.01753200

simulation compiles with CCE 19 (CPU) and GNU 12.3. Precheck passes, apart from two test_thermochem cases that fail on the Frontier login node because they compile with the system /usr/bin/gfortran (addressed by #1943). Results do not change.

Contribution Policy

We do not accept pull requests generated primarily by AI without genuine understanding or real-world usage context.

All contributions are expected to demonstrate:

  • A clear understanding of the codebase
  • Alignment with product direction
  • Thoughtful reasoning behind changes
  • Evidence of real-world usage or hands-on experience with the problem

If these expectations are not met, we would prefer to implement the changes ourselves rather than spend time reviewing low-effort submissions.


Acknowledgement

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

This PR was prepared with the assistance of an AI tool (Claude Code). We hit this while reading timings from production runs on Frontier.

PR template credit: junegunn

time_data.dat's "s/step" column holds the fastest single RK stage (one RHS
evaluation, ~1/3 of an RK3 step), and io_time_data.dat's holds the mean time
of one save. Relabel them "s/rhs" and "s/save". Columns and values are
unchanged.

Co-Authored-By: Claude <noreply@anthropic.com>
@sbryngelson
sbryngelson marked this pull request as ready for review October 8, 2026 16:48
Copilot AI balanced review requested due to automatic review settings October 8, 2026 16:48
@sbryngelson
sbryngelson merged commit 27cae2c into MFlowCode:master Oct 8, 2026
71 checks passed
@sbryngelson
sbryngelson deleted the fix-time-data-labels branch October 8, 2026 16:49

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.

🟡 Changes recommended

The new comments omit the maximum-across-ranks aggregation used for multi-rank timings.

1 open finding
What changed in this PR

Updates simulation performance-file headers to accurately identify per-RHS and per-save timings.

Changes:

  • Renames s/step headers to s/rhs and s/save.
  • Adds comments explaining both metrics.
File Description
src/​simulation/​m_start_up.fpp Relabels timing columns and documents their meanings.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/simulation/m_start_up.fpp
@codecov

codecov Bot commented Oct 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.42%. Comparing base (3dc5b2f) to head (fc629c8).
⚠️ Report is 8 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1949      +/-   ##
==========================================
- Coverage   62.64%   61.42%   -1.22%     
==========================================
  Files          86       86              
  Lines       22425    22765     +340     
  Branches     3325     3350      +25     
==========================================
- Hits        14048    13984      -64     
- Misses       6119     6321     +202     
- Partials     2258     2460     +202     

☔ 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.

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