Skip to content

Add ETA print to time-step output #1351

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wilfonba wants to merge 6 commits into
base: master
Choose a base branch
from
+40 −14
Open

Add ETA print to time-step output #1351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c720137
add ETA print to time-step output
wilfonba Apr 3, 2026
3a8b377
merge
wilfonba Apr 3, 2026
00d3a24
bug fixes from merge
wilfonba Apr 3, 2026
d6edf3b
Fix ETA off-by-one, wall_time_avg restart bug, declaration ordering
sbryngelson Apr 4, 2026
800c254
Collapse dead cfl_dt branch in wall_time_avg, fix ETA off-by-one in p…
sbryngelson Apr 4, 2026
2e9c16d
Merge branch 'master' into ETA
sbryngelson Apr 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions src/post_process/m_start_up.fpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,16 +139,27 @@ contains
impure subroutine s_perform_time_step(t_step)

integer, intent(inout) :: t_step
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 - 1 - 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
Comment on lines +142 to +153
Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review bot Apr 4, 2026

Choose a reason for hiding this comment

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

Action required

3. Eta changes outside s_perform_time_step 📎 Requirement gap ⚙ Maintainability

ETA-related functionality and timing logic were modified outside
src/simulation/m_start_up.fpp:s_perform_time_step, violating the constraint to keep the
implementation as a small localized change in that subroutine. This increases surface area and
breaks the checklist’s location/approach requirement.
Agent Prompt 
## Issue description
ETA/timing changes are spread across multiple files, but the compliance requirement constrains the ETA implementation to `src/simulation/m_start_up.fpp` within `s_perform_time_step`.

## Issue Context
The checklist requires a small, localized change using existing variables, without requiring supporting changes elsewhere for ETA.

## Fix Focus Areas
- src/post_process/m_start_up.fpp[142-153]
- src/post_process/p_main.fpp[50-56]
- src/simulation/m_time_steppers.fpp[589-590]

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

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, 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, " @ t_step = ", I8, " Time Avg = ", ES16.6, " Time/step = ", ES12.6, " ETA (HH:MM:SS) = ", I0, ":", I2.2, ":", I2.2)', &
& int(ceiling(100._wp*(real(t_step - t_step_start)/(t_step_stop - t_step_start + 1)))), &
& (t_step - t_step_start)/t_step_save + 1, (t_step_stop - t_step_start)/t_step_save + 1, t_step, &
& wall_time_avg, wall_time
& wall_time_avg, wall_time, eta_hh, eta_mm, eta_ss
end if
end if

Expand Down
11 changes: 8 additions & 3 deletions src/post_process/p_main.fpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ program p_main

implicit none

integer :: t_step !< Iterator for the main time-stepping loop
integer :: t_step, nt_step !< Iterator for the main time-stepping loop
!> Generic storage for the name(s) of the flow variable(s) that will be added to the formatted database file(s)
character(LEN=name_len) :: varname
real(wp) :: pres
Expand Down Expand Up @@ -47,8 +47,13 @@ program p_main

wall_time = abs(finish - start)

if (t_step >= 2) then
wall_time_avg = (wall_time + (t_step - 2)*wall_time_avg)/(t_step - 1)
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)
else
wall_time_avg = 0._wp
end if
Expand Down
20 changes: 15 additions & 5 deletions src/simulation/m_start_up.fpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,7 +606,8 @@ contains

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

if (cfl_dt) then
if (cfl_const_dt .and. t_step == 0) call s_compute_dt()
Expand Down Expand Up @@ -635,14 +636,23 @@ contains

if (cfl_dt) then
if (proc_rank == 0 .and. mod(t_step - t_step_start, t_step_print) == 0) then
print '(" [", I3, "%] Time ", ES16.6, " dt = ", ES16.6, " @ Time Step = ", I8, " Time Avg = ", ES16.6, " Time/step = ", ES12.6, "")', &
& int(ceiling(100._wp*(mytime/t_stop))), mytime, dt, t_step, wall_time_avg, wall_time
eta_sec = wall_time_avg*(t_stop - mytime)/max(dt, tiny(dt))
Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review bot Apr 4, 2026

Choose a reason for hiding this comment

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

Action required

1. eta_sec lacks nint steps_remaining 📎 Requirement gap ≡ Correctness

In CFL-based mode, the new ETA computation uses a real-valued division `(t_stop - mytime)/max(dt,
tiny(dt)) instead of computing integer steps_remaining = nint((t_stop - mytime)/dt)` and then
eta_seconds = wall_time_avg * steps_remaining as required. This deviates from the specified
formula and can produce inconsistent ETA estimates.
Agent Prompt 
## Issue description
CFL-mode ETA does not follow the required `steps_remaining` + `eta_seconds = wall_time_avg * steps_remaining` formula.

## Issue Context
The compliance checklist specifies:
- `steps_remaining = nint((t_stop - mytime) / dt)` (CFL mode)
- `eta_seconds = wall_time_avg * steps_remaining`

## Fix Focus Areas
- src/simulation/m_start_up.fpp[639-644]

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

eta_hh = int(eta_sec)/3600
eta_mm = mod(int(eta_sec), 3600)/60
eta_ss = mod(int(eta_sec), 60)
print '(" [", I3, "%] Time ", ES16.6, " dt = ", ES16.6, " @ Time Step = ", I8, " Time Avg = ", ES16.6, " Time/step = ", ES12.6, " ETA (HH:MM:SS) = ", I0, ":", I2.2, ":", I2.2)', &
& int(ceiling(100._wp*(mytime/t_stop))), mytime, dt, t_step, wall_time_avg, wall_time, eta_hh, eta_mm, eta_ss
Comment on lines +643 to +644
Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review bot Apr 4, 2026

Choose a reason for hiding this comment

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

Action required

2. Nonstandard eta (hh:mm:ss) label 📎 Requirement gap ≡ Correctness

The progress output prints ETA (HH:MM:SS) = ... instead of the required ETA = <duration>
convention. This breaks the specified output label/style for ETA.
Agent Prompt 
## Issue description
ETA is printed with the label `ETA (HH:MM:SS) =` rather than the required `ETA = <duration>` convention.

## Issue Context
Compliance requires the output label to match `ETA = ...`.

## Fix Focus Areas
- src/simulation/m_start_up.fpp[643-644]
- src/simulation/m_start_up.fpp[652-655]

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

end if
else
if (proc_rank == 0 .and. mod(t_step - t_step_start, t_step_print) == 0) then
print '(" [", I3, "%] Time step ", I8, " of ", I0, " @ t_step = ", I8, " Time Avg = ", ES12.6, " Time/step= ", ES12.6, "")', &
eta_sec = wall_time_avg*real(t_step_stop - 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, "%] Time step ", I8, " of ", I0, " @ t_step = ", I8, " Time Avg = ", ES12.6, " Time/step= ", ES12.6, " ETA (HH:MM:SS) = ", I0, ":", I2.2, ":", I2.2)', &
& int(ceiling(100._wp*(real(t_step - t_step_start)/(t_step_stop - t_step_start + 1)))), &
& t_step - t_step_start + 1, t_step_stop - t_step_start + 1, t_step, wall_time_avg, wall_time
& t_step - t_step_start + 1, t_step_stop - t_step_start + 1, t_step, wall_time_avg, wall_time, eta_hh, &
& eta_mm, eta_ss
end if
end if

Expand Down
4 changes: 2 additions & 2 deletions src/simulation/m_time_steppers.fpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -586,8 +586,8 @@ contains

wall_time = abs(finish - start)

if (t_step >= 2) then
wall_time_avg = (wall_time + (t_step - 2)*wall_time_avg)/(t_step - 1)
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)
Comment on lines +589 to +590
Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review bot Apr 4, 2026

Choose a reason for hiding this comment

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

Action required

4. Cfl avg uses t_step_start 🐞 Bug ≡ Correctness

In simulation, wall_time_avg is now updated using (t_step - t_step_start), but in cfl_dt mode the
main loop initializes t_step=0 while t_step_start can remain the default sentinel (-100) and is not
validated. This can make wall_time_avg (and the new ETA) drastically too small during early steps.
Agent Prompt 
## Issue description
`wall_time_avg` in `m_time_steppers` is now computed relative to `t_step_start`, but in `cfl_dt` mode `t_step` starts at 0 and `t_step_start` is not required/validated (can remain `dflt_int=-100`). This silently corrupts `wall_time_avg`, and consequently the new ETA output.

## Issue Context
In `cfl_dt` mode, the step index used for averaging should be based on the actual loop counter (`t_step` starting at 0), not an input restart index (`t_step_start`).

## Fix Focus Areas
- src/simulation/m_time_steppers.fpp[587-593]
- src/simulation/p_main.fpp[50-55]
- src/simulation/m_global_parameters.fpp[500-531]
- src/simulation/m_checker.fpp[86-94]

## What to change
- In `m_time_steppers`, compute an averaging index like:
  - `steps_since_start = t_step` when `cfl_dt`
  - `steps_since_start = t_step - t_step_start` when `.not. cfl_dt`
  and use `steps_since_start` in the `wall_time_avg` recurrence.
- Additionally (optional hardening): in initialization or input checks, enforce `t_step_start == 0` when `cfl_dt` or explicitly set it to 0 after reading the namelist to prevent future misuse elsewhere.

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

else
wall_time_avg = 0._wp
end if
Expand Down
Loading