Add ETA print to time-step output by wilfonba · Pull Request #1351 · MFlowCode/MFC

wilfonba · 2026-04-03T13:27:33Z

Description

This PR adds an ETA printout to runtime and corrects a bug in computing the average time per step in post process. Addresses #1276

Fixes #(issue)

Type of change

Bug fix
New feature

AI code reviews

Reviews are not triggered automatically. To request a review, comment on the PR:

@coderabbitai review — incremental review (new changes only)
@coderabbitai full review — full review from scratch
/review — Qodo review
/improve — Qodo code suggestions
@claude full review — Claude full review (also triggers on PR open/reopen/ready)
Add label claude-full-review — Claude full review via label

qodo-code-review · 2026-04-03T13:27:49Z

Review Summary by Qodo

Add ETA display and fix average time calculation

✨ Enhancement 🐞 Bug fix

Walkthroughs

Description

• Add ETA (hours:minutes:seconds) display to simulation progress output
• Fix average time-per-step calculation for non-CFL time-stepping mode
• Compute remaining time based on simulation progress and average step duration
• Display ETA in both CFL-based and fixed time-step simulation modes

Diagram

flowchart LR
  A["Time Step Execution"] --> B["Calculate Wall Time"]
  B --> C["Update Average Time"]
  C --> D["Compute ETA"]
  D --> E["Print Progress with ETA"]
  F["CFL Mode"] --> C
  G["Fixed Time-Step Mode"] --> C

File Changes

1. src/simulation/m_start_up.fpp ✨ Enhancement +15/-4

Add ETA display to simulation progress output

• Add ETA calculation variables (eta_hh, eta_mm, eta_ss, eta_sec)
• Calculate remaining time based on simulation progress and average step duration
• Append ETA (HH:MM:SS) format to both CFL and fixed time-step print statements
• Handle both time-based (CFL) and step-based simulation modes

src/simulation/m_start_up.fpp

2. src/post_process/m_start_up.fpp ✨ Enhancement +15/-4

Add ETA display to post-process time-step output

• Add ETA calculation variables for post-processing time-step output
• Calculate remaining time for both CFL and non-CFL post-processing modes
• Append ETA (HH:MM:SS) to print statements for progress tracking
• Support ETA display in data saving operations

src/post_process/m_start_up.fpp

3. src/simulation/m_time_steppers.fpp 🐞 Bug fix +11/-3

Fix average time calculation for time-stepping modes

• Fix average time-per-step calculation to distinguish between CFL and fixed time-step modes
• For CFL mode: use absolute time-step count from simulation start
• For fixed time-step mode: use relative time-step count from t_step_start
• Ensure correct averaging denominator based on simulation mode

src/simulation/m_time_steppers.fpp

View more (1)

4. src/post_process/p_main.fpp 🐞 Bug fix +7/-2

Fix average time calculation in post-process main

• Introduce nt_step variable to track effective time-step count
• Differentiate between CFL mode (count from n_start) and fixed mode (count from t_step_start)
• Fix average time-per-step calculation using correct step count
• Ensure consistent time averaging across different simulation modes

src/post_process/p_main.fpp

qodo-code-review · 2026-04-03T13:27:50Z

Code Review by Qodo

🐞 Bugs (4) 📘 Rule violations (1) 📎 Requirement gaps (0) 🎨 UX Issues (0)

1. ETA output indentation wrong 📘 Rule violation ⚙ Maintainability

Description

The newly added ETA/average-time logic uses 4/8-space indentation (e.g., lines beginning with 8
spaces inside blocks) rather than the required 2-space indentation. This violates the project’s
Fortran style compliance requirement and reduces formatting consistency/enforceability by automated
style checks.

Code

src/post_process/m_start_up.fpp[R142-153]

+        integer                :: eta_hh, eta_mm, eta_ss
+        real(wp)               :: eta_sec

        if (proc_rank == 0) then
            if (cfl_dt) then
-                print '(" [", I3, "%]  Saving ", I8, " of ", I0, " Time Avg = ", ES16.6,  " Time/step = ", ES12.6, "")', &
-                    & int(ceiling(100._wp*(real(t_step - n_start)/(n_save)))), t_step, n_save, wall_time_avg, wall_time
+                eta_sec = wall_time_avg*real(n_save - t_step, wp)
+                eta_hh = int(eta_sec)/3600
+                eta_mm = mod(int(eta_sec), 3600)/60
+                eta_ss = mod(int(eta_sec), 60)
+                print '(" [", I3, "%]  Saving ", I8, " of ", I0, " Time Avg = ", ES16.6,  " Time/step = ", ES12.6, " ETA (HH:MM:SS)  = ", I0, ":", I2.2, ":", I2.2)', &
+                    & int(ceiling(100._wp*(real(t_step - n_start)/(n_save)))), t_step, n_save, wall_time_avg, wall_time, eta_hh, &
+                    & eta_mm, eta_ss

Evidence
PR Compliance ID 8 requires 2-space indentation. The added ETA/avg-time blocks in multiple modified
files are indented with more than 2 spaces (e.g.,         integer ...,         if (cfl_dt) then,
etc.), demonstrating noncompliance on newly changed lines.
CLAUDE.md
src/post_process/m_start_up.fpp[139-163]
src/post_process/p_main.fpp[33-59]
src/simulation/m_start_up.fpp[609-662]
src/simulation/m_time_steppers.fpp[588-602]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Newly added/modified Fortran code blocks are indented with 4/8 spaces rather than the required 2-space indentation.

## Issue Context
PR Compliance ID 8 requires 2-space indentation for Fortran style consistency.

## Fix Focus Areas
- src/post_process/m_start_up.fpp[139-163]
- src/post_process/p_main.fpp[33-59]
- src/simulation/m_start_up.fpp[609-662]
- src/simulation/m_time_steppers.fpp[588-602]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. Undeclared nt_step 🐞 Bug ≡ Correctness

Description

In post-process p_main, nt_step is assigned under implicit none but is never declared in the
program scope, causing compilation to fail. This will block building/running post_process.

Code

src/post_process/p_main.fpp[R50-56]

+        if (cfl_dt) then
+            nt_step = t_step - n_start + 1
+        else
+            nt_step = (t_step - t_step_start)/t_step_save + 1
+        end if
+        if (nt_step >= 2) then
+            wall_time_avg = (wall_time + (nt_step - 2)*wall_time_avg)/(nt_step - 1)

Evidence
implicit none requires all variables be explicitly declared; p_main declares t_step and others
but not nt_step, yet assigns/reads nt_step for the running-average update.
src/post_process/p_main.fpp[11-59]
src/post_process/m_global_parameters.fpp[69-81]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`src/post_process/p_main.fpp` assigns to `nt_step` under `implicit none`, but `nt_step` is not declared in the program scope (and is not present in post_process global parameters), causing a compilation error.

### Issue Context
The new averaging logic computes a derived step count (`nt_step`) and then uses it to update `wall_time_avg`.

### Fix Focus Areas
- src/post_process/p_main.fpp[11-59]
- src/post_process/m_global_parameters.fpp[69-81]

### Suggested fix
Add a local declaration like `integer :: nt_step` in `program p_main` (or add it to an appropriate module if it is intended to be a shared global).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

3. Duplicate i declaration 🐞 Bug ≡ Correctness

Description

In simulation s_perform_time_step, variable i is declared twice in the same scope, which is a
Fortran compile-time error. This will prevent the simulation from compiling.

Code

src/simulation/m_start_up.fpp[R612-616]

+        integer                 :: i, eta_hh, eta_mm, eta_ss
+        real(wp)                :: eta_sec
        integer, intent(inout)  :: t_step
        real(wp), intent(inout) :: time_avg
        integer                 :: i

Evidence

The subroutine declares integer :: i, eta_hh, ... and then declares integer :: i again a few
lines later in the same subroutine scope.

src/simulation/m_start_up.fpp[609-617]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`src/simulation/m_start_up.fpp:s_perform_time_step` declares `i` twice, which is illegal in Fortran and fails compilation.

### Issue Context
The ETA variables were added by extending the existing local declarations, but an existing `integer :: i` declaration remained.

### Fix Focus Areas
- src/simulation/m_start_up.fpp[609-617]

### Suggested fix
Keep a single declaration for `i` (either in the combined declaration list or as its own line) and remove the other one.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

4. Wrong non-CFL averaging 🐞 Bug ≡ Correctness

Description

In s_tvd_rk (simulation), the non-CFL wall_time_avg update uses an incorrect step offset
relative to t_step_start, delaying the average by an extra step and undercounting thereafter. This
makes the printed Time Avg/ETA incorrect for non-CFL runs.

Code

src/simulation/m_time_steppers.fpp[R597-601]

+            if (t_step - t_step_start >= 2) then
+                wall_time_avg = (wall_time + (t_step - t_step_start - 2)*wall_time_avg)/(t_step - t_step_start - 1)
+            else
+                wall_time_avg = 0._wp
+            end if

Evidence

The main loop starts with t_step = t_step_start, so on the second executed step `t_step -
t_step_start == 1; however the new condition requires >= 2, forcing wall_time_avg=0` for the
second step. The update formula also subtracts 2 and divides by (…-1), effectively skipping two
steps instead of mirroring the CFL branch’s "skip first step" behavior with an offset.

src/simulation/m_time_steppers.fpp[588-602]
src/simulation/p_main.fpp[50-84]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The non-CFL `wall_time_avg` running average in `s_tvd_rk` uses an off-by-one offset based on `t_step_start`, causing the average/ETA to be wrong for the first few steps and biased thereafter.

### Issue Context
The CFL branch uses a running average that effectively excludes the first step (`t_step==1`) and averages subsequent steps. The non-CFL branch should mirror that behavior but with an index offset of `t_step_start`.

### Fix Focus Areas
- src/simulation/m_time_steppers.fpp[588-602]
- src/simulation/p_main.fpp[50-84]

### Suggested fix
Replace the non-CFL branch with an offset-correct version, e.g.:
- Let `k = t_step - t_step_start` (number of steps since start).
- If `k >= 1`: `wall_time_avg = (wall_time + (k - 1)*wall_time_avg)/k`, else `wall_time_avg = 0._wp`.
This matches the CFL branch’s intent (exclude the first step) but relative to `t_step_start`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

5. Post-process ETA off-by-one 🐞 Bug ≡ Correctness

Description

Post-process ETA computation counts one extra remaining save in both CFL and fixed-dt modes, so ETA
never reaches 0 at the final processed step. This makes the newly added ETA printout incorrect.

Code

src/post_process/m_start_up.fpp[R147-156]

+                eta_sec = wall_time_avg*real(n_save - t_step, wp)
+                eta_hh = int(eta_sec)/3600
+                eta_mm = mod(int(eta_sec), 3600)/60
+                eta_ss = mod(int(eta_sec), 60)
+                print '(" [", I3, "%]  Saving ", I8, " of ", I0, " Time Avg = ", ES16.6,  " Time/step = ", ES12.6, " ETA (HH:MM:SS)  = ", I0, ":", I2.2, ":", I2.2)', &
+                    & int(ceiling(100._wp*(real(t_step - n_start)/(n_save)))), t_step, n_save, wall_time_avg, wall_time, eta_hh, &
+                    & eta_mm, eta_ss
            else
-                print '(" [", I3, "%]  Saving ", I8, " of ", I0, " @ t_step = ", I8, " Time Avg = ", ES16.6,  " Time/step = ", ES12.6, "")', &
+                eta_sec = wall_time_avg*real((t_step_stop - t_step)/t_step_save + 1, wp)
+                eta_hh = int(eta_sec)/3600

Evidence

In CFL mode, the loop exits when t_step == n_save - 1, but ETA uses n_save - t_step, yielding 1
step remaining at the last iteration. In fixed-dt mode, the loop exits when t_step == t_step_stop,
but ETA uses (...)+1, also yielding 1 remaining step at the end.

src/post_process/m_start_up.fpp[145-163]
src/post_process/p_main.fpp[61-71]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
ETA in post-process includes an extra remaining step in both CFL and fixed-dt modes, so the final step still reports a non-zero ETA.

### Issue Context
The post-process loop termination conditions define the last processed step:
- CFL: exit when `t_step == n_save - 1`
- Fixed dt: exit when `t_step == t_step_stop`
So remaining steps at the last iteration should be 0.

### Fix Focus Areas
- src/post_process/m_start_up.fpp[145-163]
- src/post_process/p_main.fpp[61-71]

### Suggested fix
Adjust remaining-step calculations:
- CFL: `remaining = (n_save - 1) - t_step`
- Fixed dt: `remaining = (t_step_stop - t_step)/t_step_save`
Then compute `eta_sec = wall_time_avg * real(remaining, wp)`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

ⓘ The new review experience is currently in Beta. Learn more

coderabbitai · 2026-04-03T13:31:22Z

📝 Walkthrough

Walkthrough

This pull request adds estimated time remaining (ETA) display to simulation progress output and refines the wall-time averaging logic across multiple time-stepping and post-processing modules. ETA is computed and formatted as HH:MM:SS using local variables in progress print statements. The wall-time averaging calculation is modified to depend on whether CFL-based variable time stepping is enabled, with different averaging windows and reset conditions applied to each mode. Changes span four files with a total of approximately 48 additions and 13 deletions.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	PR description is missing key required sections including Testing, Checklist items, and GPU changes acknowledgment despite modifications to src/simulation/.	Add Testing section explaining how changes were validated, complete the Checklist (test/documentation updates), and address GPU changes section since src/simulation/ was modified.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: adding ETA output to time-step runtime output, which is the primary feature implemented across multiple files.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 2

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: e47f0737-3eea-432c-8f47-db3514729cd3

📥 Commits

Reviewing files that changed from the base of the PR and between 2d15ba9 and 3a8b377.

📒 Files selected for processing (4)

src/post_process/m_start_up.fpp
src/post_process/p_main.fpp
src/simulation/m_start_up.fpp
src/simulation/m_time_steppers.fpp

src/post_process/m_start_up.fpp

src/simulation/m_start_up.fpp

src/post_process/m_start_up.fpp

src/post_process/p_main.fpp

src/simulation/m_start_up.fpp

wilfonba added 2 commits April 3, 2026 09:21

add ETA print to time-step output

c720137

merge

3a8b377

coderabbitai bot reviewed Apr 3, 2026

View reviewed changes

src/post_process/m_start_up.fpp Show resolved Hide resolved

src/simulation/m_start_up.fpp Outdated Show resolved Hide resolved

qodo-code-review bot reviewed Apr 3, 2026

View reviewed changes

src/post_process/m_start_up.fpp Show resolved Hide resolved

src/post_process/p_main.fpp Show resolved Hide resolved

src/simulation/m_start_up.fpp Outdated Show resolved Hide resolved

wilfonba marked this pull request as draft April 3, 2026 13:32

bug fixes from merge

00d3a24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ETA print to time-step output#1351

Add ETA print to time-step output#1351
wilfonba wants to merge 3 commits intoMFlowCode:masterfrom
wilfonba:ETA

wilfonba commented Apr 3, 2026 •

edited

Loading

Uh oh!

qodo-code-review bot commented Apr 3, 2026

Uh oh!

qodo-code-review bot commented Apr 3, 2026 •

edited

Loading

Uh oh!

coderabbitai bot commented Apr 3, 2026 •

edited

Loading

Walkthrough

❌ Failed checks (1 warning)

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

1 participant

Conversation

wilfonba commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

AI code reviews

Uh oh!

qodo-code-review bot commented Apr 3, 2026

Review Summary by Qodo

Walkthroughs

File Changes

Uh oh!

qodo-code-review bot commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

coderabbitai bot commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

❌ Failed checks (1 warning)

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

1 participant

wilfonba commented Apr 3, 2026 •

edited

Loading

qodo-code-review bot commented Apr 3, 2026 •

edited

Loading

coderabbitai bot commented Apr 3, 2026 •

edited

Loading