Fix hist_hi and iteration outputs
#2534
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
include/bout/solver.hxx
Outdated
| return 0; | ||
| } | ||
| }; | ||
| SolverMonitor solver_monitor; |
There was a problem hiding this comment.
warning: member variable solver_monitor has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
SolverMonitor solver_monitor;
^There was a problem hiding this comment.
This was deliberate - I used it to make the SolverTest.RemoveMonitor test work by providing a getter for solver_monitor in the FakeSolver class.
There was a problem hiding this comment.
Could maybe make solver_monitor private and add a protected getter?
|
I like removing the duplicated Instead of adding a new monitor, does it make sense to do the iteration increment in the output monitor itself? |
That does seem nicer! Just need to add a method to |
|
We could also make |
|
Should I rebase this branch to get rid of stuff that was added and then removed (e.g. |
include/bout/solver.hxx
Outdated
| /// Add one to the iteration count, used by BoutMonitor, but could be called by a | ||
| //user-defined monitor (if `bout_run()` is not used) | ||
| void incrementIterationCounter() { ++iteration; } |
There was a problem hiding this comment.
Small suggestion:
| /// Add one to the iteration count, used by BoutMonitor, but could be called by a | |
| //user-defined monitor (if `bout_run()` is not used) | |
| void incrementIterationCounter() { ++iteration; } | |
| /// Add one to the iteration count, used by BoutMonitor, but could be called by a | |
| /// user-defined monitor (if `bout_run()` is not used). | |
| /// Returns the incremented value | |
| int incrementIterationCounter() { return ++iteration; } |
means below we can do:
// Get updated iteration count
iteration = solver->incrementIterationCounter();There was a problem hiding this comment.
Nice! I'll put that in.
Sure, that might be nice -- but it's not a blocker if you don't :) |
|
Tests are failing because there's a use of |
It looked to me as though the uses of |
Previously, the `iteration` saved into dump files was inconsistent with `hist_hi`: in a simulation that is not restarted (so starts from iteration 0), `iteration` would always be 1 less than `hist_hi`. `hist_hi` counts the number of output steps that have been completed, which seems more sensible, so this commit adds 1 to the value of `iteration` that is saved. Note this also makes the value that is saved the same as the value that is printed to stdout at the same time.
Make `Solver::iteration` private, and add methods to get or increment its value. Allows the iteration counter to be updated directly in `BoutMonitor::call()`. Previously, `Solver::iteration` was updated in the implementations of `Solver::run()`, but if there is any monitor that is called more frequently than the output monitor, this meant that `iteration` counted the number of calls to the more frequent monitor, not the number of output steps. Not an issue now, since it is incremented in `BoutMonitor`, which is the one that does the output.
SlepcSolver uses `Solver::iteration`. Adding the protected `Solver::resetIterationCounter()` method allows `Solver::iteration` to be private, while allowing `SlepcSolver` to work.
johnomotani
force-pushed
the
fix-iteration-counts
branch
from
bb56d95
to
b5b320e
Compare
We do indeed have a |
Need to use `++iteration`, not `iteration++`, so that the returned value is the after-increment value. Co-authored-by: Peter Hill <peter.hill@york.ac.uk>
This PR fixes two issues that I think are bugs:
hist_hicounts the number of steps of the most frequentMonitor, which is not necessarily the output monitor. I think we always want to count the number of output steps. Fixed in this PR by moving the increment ofSolver::iterationto aSolverMonitor, which is called with the same frequency as the output monitor. TheSolverMonitoris called before writing dump and output files, sohist_hibehaves the same as at present when the output monitor is the most frequent monitor.iteration, as saved to the dump files, is too low by 1. I think the value should be: the same ashist_hiwhen the simulation was started from iteration 0 (i.e. not restarted); the same as the value printed to stdout at the same time as it is being saved. Fixed by 6b8eff8.Question: is issue 2 actually slightly more general, and we should actually call
call_monitorswith theiterargument one greater (so that it counts the number of iterations that have been completed)? Proposed change to do this in #2535.