Fix issues with history output - #624
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses incorrect convergence history reporting in multi-equation simulations (Fixes #615) by restructuring when/where history rows are emitted, and by reformatting the history table for consistent fixed-width alignment.
Changes:
- Introduce
output::output_header()and refactoroutput::output_result()to generate aligned, fixed-width history output (including improved separator handling). - Move history row emission into
Integrator::step()so every Newton iteration (including the converged one) is printed exactly once, with ansflag when VTU output is scheduled. - Simplify VTU-save decision logic in
main.cppand pass it intoIntegrator::step(save_vtu).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Code/Source/solver/output.h | Adds output_header() and updates output_result() API/documentation. |
| Code/Source/solver/output.cpp | Implements header printing + fixed-width row formatting and separator generation. |
| Code/Source/solver/main.cpp | Computes save_vtu, passes it to Integrator::step(), and streamlines VTU write logic. |
| Code/Source/solver/Integrator.h | Extends step() signature to accept save_results flag (defaulted). |
| Code/Source/solver/Integrator.cpp | Emits one history row per Newton iteration (including converged) from within step(). |
| Code/Source/solver/initialize.cpp | Switches history initialization from output_result(co=1) to output_header(). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #624 +/- ##
=======================================
Coverage 72.86% 72.86%
=======================================
Files 258 258
Lines 39500 39490 -10
Branches 6730 6726 -4
=======================================
- Hits 28780 28773 -7
+ Misses 10477 10474 -3
Partials 243 243 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kko27
left a comment
There was a problem hiding this comment.
Thanks so much @michelebucelli! The changes look great! Have two minor comments.
| bool l1 = false; | ||
| bool l2 = false; |
There was a problem hiding this comment.
Is there a possibility of also replacing these boolean flags with more meaningful variable names, or removing them? Not sure if that increases the scope of this PR.
There was a problem hiding this comment.
Done, as follows:
l1was only used in a statement likel1 = expression_A; if (expression_B && l1) { ... }. I removed the variablel1entirely, and replaced that statement withif (expression_B && expression_A) { ... }. Since the two expressions were slightly obscure due to the variables involved having somewhat cryptic names (to me at least), I included comments to explain the meaning of both (I'd rather not go on a variable renaming spree at this time 😅 ).l2was used similarly, although its value was printed by a debug statement as well so it couldn't be removed without changing behavior. I moved its declaration down to where it is used, and gave it the more explicit nameis_restart_output_step.- There was another
l1variable, declared in thewhileloop (and thus hiding thel1variable here). I also renamed it toreached_stop_time_step.
Conceivably there's a bit more cleanup to be done in the main function code along these lines (making variables as local as possible, giving more expressive names, removing unused variables), but maybe we can save it for a more scoped issue.
Fixes #615 about incorrect reporting of iterations in multi-equation simulations. Also adds some cosmetic improvements to the history table. See the issue for details.
This has no effect on calculations, but only on convergence history reporting.
Code of Conduct & Contributing Guidelines