Encapsulate Newton iteration in Integrator class (#442)#450
Encapsulate Newton iteration in Integrator class (#442)#450Eleven7825 wants to merge 51 commits intoSimVascular:mainfrom
Conversation
- Add CMake-generated files (CMakeCache.txt, CMakeFiles/, Makefile, etc.) - Add build directories (*-build/, *-prefix/) - Add compiled outputs (Code/bin/, Code/lib/) - Add generated headers - Add vim swap files (*.swp, *.swo, *~) Related to SimVascular#442
- Create integrator.h and integrator.cpp with Integrator class - Encapsulates solution variables (Ag, Yg, Dg) - Handles Newton iteration loop - Manages linear system assembly and solve - Applies boundary conditions - Update CMakeLists.txt to include new source files This is the first step toward implementing partitioned FSI as described in SimVascular#442
- Replace Newton iteration loop with Integrator::step() call - Remove local Ag, Yg, Dg arrays (now managed by Integrator) - Simplify iterate_solution() function - Add integrator.h include Addresses SimVascular#442: Encapsulate Newton iteration in main.cpp
The iEqOld variable is needed for output::output_result() calls after the Newton iteration completes. This variable tracks which equation was solved in the last iteration.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #450 +/- ##
==========================================
+ Coverage 67.84% 67.93% +0.08%
==========================================
Files 168 170 +2
Lines 32797 32946 +149
Branches 5768 5766 -2
==========================================
+ Hits 22252 22382 +130
- Misses 10408 10427 +19
Partials 137 137 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mrp089
left a comment
There was a problem hiding this comment.
Thanks for taking a crack at this, @Eleven7825! I've added some comments.
|
@Eleven7825 This is a big change and needs to be discussed with the solver team. |
|
Sounds good! Who do we talk to and when? |
|
Maybe we could bring this up this Friday during the refactory meeting? |
|
@mrp089 The initial refactoring effort will be the design and implementation of some sort of solver class to encapsulate solver data and support G&R. We can start this discussion in the first |
|
@dcodoni suggested removing |
- Rename integrator.h to Integrator.h - Rename integrator.cpp to Integrator.cpp - Update all #include statements in main.cpp and Integrator.cpp - Update CMakeLists.txt reference Addresses PR SimVascular#450 review feedback on file naming convention.
- Use JavaDoc-style /** */ comments throughout Integrator.h - Add @brief, @param, and @return tags following svZeroDSolver conventions - Document all public methods and private members - Improve descriptions for solution variables (Ag, Yg, Dg) and helper methods Addresses PR SimVascular#450 review feedback on Doxygen documentation format.
- Add istr_ as private member variable in Integrator class - Update step() method to set istr_ instead of using local variable - Replace all istr references with istr_ throughout the code - Simplify debug write statements in apply_boundary_conditions() This change enables individual helper functions to access istr_ for their own debug outputs, preparing for the next step of moving write statements into individual functions. Addresses PR SimVascular#450 review feedback on variable management.
- Move all .write() debug statements from step() to their respective helper functions - initiator_step() now handles its own debug output (Ag, Yg, Dg, Yn) - allocate_linear_system() now handles Val output - set_body_forces() now handles Val output - assemble_equations() now handles R and Val output - apply_boundary_conditions() now handles Val, R, Yg, Dg output - solve_linear_system() now handles Val and R output - corrector_and_check_convergence() now handles Yn output This makes step() cleaner and more focused on orchestration, while each helper function manages its own debug logging. All functions now use the istr_ class member variable for consistent debug naming. Addresses PR SimVascular#450 review feedback on code organization.
…oop' - Rename inner_count_ to newton_count_ throughout Integrator class - Update comments to use 'Newton iteration' instead of 'inner loop' - Update debug messages to show 'Newton Iteration' instead of 'Inner Loop' This makes the terminology more precise and consistent with standard computational mechanics nomenclature, clarifying that we're referring to the Newton iteration within each time step (as opposed to the outer time loop). Addresses PR SimVascular#450 review feedback on terminology consistency.
- Move debug print statements from step() into individual helper functions - Each function now handles its own debug output: * initiator_step() * allocate_linear_system() * set_body_forces() * assemble_equations() * apply_boundary_conditions() * update_residual_arrays() * solve_linear_system() * corrector_and_check_convergence() - Makes step() method cleaner and more focused on orchestration - Each helper function is now self-contained with its own debug logging This addresses PR SimVascular#450 review feedback on code organization.
|
Hi Martin, I have moved the debug messages into respective functions, and the code is ready for review. As for your last comment, I think we should address it in another issue and PR. |
Moved all pic namespace functions (picp, pici, picc, pic_eth) into the Integrator class as member functions: - picp -> predictor() (public method, called from main.cpp) - pici -> pici() (private method) - picc -> picc() (private method) - pic_eth -> pic_eth() (private method) Updated all call sites to use the new member functions. Removed pic.h and pic.cpp files and updated CMakeLists.txt. Related to GitHub issue SimVascular#459.
Move time integration variables An (acceleration), Dn (displacement), and Yn (velocity) from the global ComMod structure into the Integrator class. These variables are now passed as function parameters throughout the codebase instead of accessing them through ComMod. Changes include: - Add An_, Dn_, Yn_ members to Integrator with public getters - Initialize from Ao, Do, Yo in Integrator::initialize_arrays() - Update 19 files to pass these variables as Array<double>& parameters - Remove An, Dn, Yn declarations from ComMod.h
This commit completes the extraction of time-level variables from the global ComMod structure to the Integrator class. Ao, Do, Yo (old acceleration, displacement, velocity at time level n) join An, Dn, Yn as Integrator members. Key changes: - ComMod.h: Removed Ao, Do, Yo member variables - Integrator.h/cpp: Added Ao_, Do_, Yo_ members with move constructor and getters - Simulation.h/cpp: Added initialize_integrator() to transfer ownership - initialize.cpp: Made Ao, Do, Yo local variables, moved to Integrator - Updated 25+ files with function signatures to pass Ao/Do/Yo parameters - Fixed default parameter placement (declarations only, not definitions) All time integration state is now encapsulated in the Integrator class, improving modularity and preparing for future multi-physics enhancements.
mrp089
left a comment
There was a problem hiding this comment.
Great job, @shiyi! I definitely appreciate the missing lines in ComMod.h!
I had mostly minor comments. Two major things I noticed:
- Does the remeshing still work as intended? I'm not sure if this is covered by a test.
- It seems in
initialize.cppthere are a few things that are not adapted yet to then's, potentiallyset_bc::set_bc_dirandtxt_ns::txt.
We should look at the coverage report carefully for this one and make sure all the changed lines are covered by a test.
…nd integration functions Following the extraction of Ao/Do/Yo from ComMod to Integrator class, this commit completes the parameter threading work by updating all functions that perform face integration or boundary condition operations to accept and pass Do/Dn parameters. Changes include: Boundary Condition Functions: - bc_ini(): Add Do parameter, update 3 integ() calls for parabolic profiles and flux normalization - set_bc_cmm(), set_bc_cmm_l(): Add Do parameter for CMM boundary conditions - set_bc_neu(), set_bc_neu_l(): Add Do parameter for Neumann boundary conditions - set_bc_dir_w(), set_bc_dir_wl(): Add Do parameter for Dirichlet wall conditions - set_bc_trac_l(): Add Do parameter for traction boundary conditions - set_bc_rbnl(): Add Do parameter for Robin boundary conditions - set_bc_cpl(): Update to pass Do to bc_ini() for coupled BC area recalculation FSI and Mesh Functions: - fsi_ls_upd(): Add both Dn and Do parameters for level set updates - fsi_ls_ini(): Add Do parameter for FSI initialization - b_assem_neu_bc(), b_neu_folw_p(): Add Do parameter for Neumann BC assembly RIS (Reduced Immersed Surface) Functions: - ris_meanq(): Add Do parameter, update 2 integ() calls for pressure/flow computation - ris0d_status(): Add Do parameter, update 3 integ() calls for 0D coupling - ris_resbc(), setbc_ris(), ris0d_bc(): Add Do parameter for RIS boundary conditions CMM (Coupled Momentum Method): - cmm_b(): Add Do parameter, update nn::gnnb() call with full parameters Integration Functions: - All face-based integ() calls updated to pass Do parameter via pointer - Updated calls to use proper signatures with pFlag, cfg, Dn, and Do parameters - Enhanced error reporting in nn::gnnb() to include face name, mesh name, and element Main Integration Loop: - Integrator::iterate(): Updated all set_bc calls to pass Do_ parameter - main.cpp: Updated ris_meanq() and ris0d_status() calls to pass Do
This commit fixes systematic issues with displacement parameter passing throughout the solver: 1. Corrected parameter confusion in set_bc.cpp where An/Ao were incorrectly used instead of Dn/Do 2. Replaced nullptr with proper Dn/Do parameters in 14 integ() calls across ris.cpp, txt.cpp, baf_ini.cpp, and set_bc.cpp 3. Fixed critical bug in txt.cpp:520 where missing pFlag parameter caused &Do to be misinterpreted as boolean 4. Enhanced error messages in all_fun.cpp to identify exact failure locations These fixes resolve 12 test failures in moving mesh simulations (genBC and RIS tests).
…n7825/svMultiPhysics into feature/issue-442-integrator-class Please enter a commit message to explain why this merge is necessary,
|
@mrp089 , I have addressed all the comments. Those changes are ready for review. |
mrp089
left a comment
There was a problem hiding this comment.
Looking good, @Eleven7825! I started annotating the first few files, but then noticed two global changes that we should make first:
- Make
SolutionStates& solutionsaconstargument wherever possible (to indicate that the functions don't change the solution) - Don't pass individual solution arrays anywhere. Always pass the
solutionobject down to the final function call
… to gnnb/cep_integ Address PR review comments: make SolutionStates& const wherever the function only reads solution arrays, update nn::gnnb to accept const SolutionStates& instead of individual Dn/Do pointers, update cep_integ to accept SolutionStates& instead of individual arrays, and remove leftover pointer aliases that were only needed for the old gnnb interface. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@mrp089 I tried to |
mrp089
left a comment
There was a problem hiding this comment.
A couple more solutions to be passed. We don't want to pass individual solution arrays anymore.
Port explicit geometric coupling (expl_geom_cpl) from main's pic.cpp into refactored Integrator::corrector() method. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…itespace-only changes for easier comparison
|
@Eleven7825 What's up with the last commit; not sure what |
|
Our existing code has lots of spurious whitespace, such as at the end of a line or on an empty line. When using tools like @claude for refactoring, these whitespaces often get deleted. This makes it hard to review (especially in a large PR like this) because lines appear edited but have no change in function. My commit dbe71cc restored these spurious whitespaces to make the reviewers' job easier. Best practice is to use an automatic linter to handle code formatting once and for all, automatically (I reopened #96). |
I participated in the refactoring, so my review should be replaced by someone else's
|
This is ready from our side. @ktbolt, @aabrown100-git, please give it a review. Thank you! |
There was a problem hiding this comment.
Pull request overview
Refactors the solver time-stepping workflow to encapsulate the Newton iteration loop into a new Integrator class and to centralize solution variable handling via SolutionStates, as groundwork for future partitioned FSI (separate integrators per domain).
Changes:
- Introduced
SolutionStatesto carry old/current solution arrays and threaded it through BC, post-processing, IO, and mesh utilities. - Added
Integratorclass to own the Newton loop and generalized-alpha working arrays (Ag/Yg/Dg), and updatedmain.cppto delegate predictor + Newton step. - Removed
Ao/An/Yo/Yn/Do/DnfromComModand updated build + related call sites accordingly.
Reviewed changes
Copilot reviewed 43 out of 44 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Code/Source/solver/vtk_xml.h | Updates VTU writer API to accept SolutionStates. |
| Code/Source/solver/vtk_xml.cpp | Uses SolutionStates for VTU output and routes post-processing calls accordingly. |
| Code/Source/solver/Vector.h | Adds <cstring> include (likely for mem ops used in Vector implementation). |
| Code/Source/solver/uris.h | Updates URIS APIs to accept SolutionStates. |
| Code/Source/solver/uris.cpp | Switches URIS computations to read state from SolutionStates and passes it into integrations. |
| Code/Source/solver/txt.h | Updates TXT output APIs to accept SolutionStates. |
| Code/Source/solver/txt.cpp | Switches TXT output and integral writers to use SolutionStates. |
| Code/Source/solver/SolutionStates.h | Adds new Solution / SolutionStates containers for old/current solution arrays. |
| Code/Source/solver/Simulation.h | Adds integrator ownership/accessors and initialize_integrator(). |
| Code/Source/solver/Simulation.cpp | Implements integrator initialization and guarded accessor. |
| Code/Source/solver/set_bc.h | Threads SolutionStates through BC APIs; changes set_bc_dir signature. |
| Code/Source/solver/set_bc.cpp | Updates BC implementations to use SolutionStates and updated integration/normal APIs. |
| Code/Source/solver/ris.h | Updates RIS APIs to accept SolutionStates. |
| Code/Source/solver/ris.cpp | Switches RIS logic to use SolutionStates (including updater/status paths). |
| Code/Source/solver/remesh.cpp | Removes clearing of ComMod solution arrays now managed elsewhere. |
| Code/Source/solver/read_msh.h | Threads SolutionStates into mesh quality checks (mvMsh displacement access). |
| Code/Source/solver/read_msh.cpp | Uses SolutionStates displacement when computing mesh properties. |
| Code/Source/solver/post.h | Updates post-processing APIs to accept SolutionStates. |
| Code/Source/solver/post.cpp | Switches post-processing to read from SolutionStates; updates bin2vtk path to use temp states. |
| Code/Source/solver/pic.h | Removes legacy PIC header (predictor/initiator/corrector moved into Integrator). |
| Code/Source/solver/output.h | Threads SolutionStates into restart/results writing. |
| Code/Source/solver/output.cpp | Uses SolutionStates arrays for restart/results output. |
| Code/Source/solver/nn.h | Updates gnnb() signature to accept SolutionStates for configuration-based geometry. |
| Code/Source/solver/nn.cpp | Uses SolutionStates displacement in gnnb() (mvMsh and timestep configs). |
| Code/Source/solver/mesh.h | Threads SolutionStates into mesh construction. |
| Code/Source/solver/mesh.cpp | Uses SolutionStates displacement instead of ComMod.Do. |
| Code/Source/solver/main.cpp | Replaces inline Newton loop with Integrator calls; routes outputs/BC/RIS/URIS via SolutionStates. |
| Code/Source/solver/Integrator.h | Adds new Integrator interface encapsulating Newton iteration + generalized-alpha state. |
| Code/Source/solver/Integrator.cpp | Implements predictor/initiator/corrector and Newton loop formerly in main.cpp/PIC routines. |
| Code/Source/solver/initialize.h | Updates init APIs to fill SolutionStates rather than ComMod solution arrays. |
| Code/Source/solver/initialize.cpp | Builds initial SolutionStates, applies BCs, initializes integrator, updates restart/init paths. |
| Code/Source/solver/eq_assem.h | Threads SolutionStates into assembly helpers and global assembly. |
| Code/Source/solver/eq_assem.cpp | Passes SolutionStates into mesh construction and BC-related normal computations. |
| Code/Source/solver/ComMod.h | Removes Ao/An/Yo/Yn/Do/Dn members from ComMod. |
| Code/Source/solver/cmm.h | Threads SolutionStates into CMM boundary routine signature. |
| Code/Source/solver/cmm.cpp | Passes SolutionStates into nn::gnnb() for consistent geometry configuration. |
| Code/Source/solver/CMakeLists.txt | Adds Integrator.* and removes pic.* from build. |
| Code/Source/solver/cep_ion.h | Updates CEP integration API to accept SolutionStates. |
| Code/Source/solver/cep_ion.cpp | Uses SolutionStates (and updated post API) for CEP state integration. |
| Code/Source/solver/baf_ini.h | Threads SolutionStates into face/bc initialization and FSILS init APIs. |
| Code/Source/solver/baf_ini.cpp | Updates BAF init to pass SolutionStates into face/bc init and coupling init calls. |
| Code/Source/solver/all_fun.h | Threads SolutionStates through integration overloads (mvMsh displacement access). |
| Code/Source/solver/all_fun.cpp | Uses SolutionStates displacement in volume/face integrals and normal computations. |
| .gitignore | Expands ignored build artifacts and generated outputs/headers. |
Comments suppressed due to low confidence (2)
Code/Source/solver/Integrator.cpp:365
res_is a persistent member but is never reset inupdate_residual_arrays(). Only entries touched bylsPtrare overwritten, so stale values from previous equations/iterations can leak intols_solve()for indices not set this call. Clearres_(or at least the active indices) at the start of this function, alongsideincL_ = 0.
Code/Source/solver/Integrator.cpp:464- Inside the
#ifdef debug_picpblock,coefis logged before it is declared/initialized (dmsg << "coef: " << coef;comes beforedouble coef = ...). Ifdebug_picpis enabled this won’t compile (or may print an indeterminate value if an outercoefexists). Move the debug print after thecoefdefinition or logeq.gamonly.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Replicates 'SUBROUTINE BAFINI()' defined in BAFINIT.f | ||
| // | ||
| void baf_ini(Simulation* simulation) | ||
| void baf_ini(Simulation* simulation, SolutionStates& solutions) |
There was a problem hiding this comment.
Just a thought. Does it make sense to make SolutionStates a member of Simulation? Seems like the Simulation class is meant to contain all high-level data structures necessary for running the simulation, and I think SolutionStates qualifies as one of those.
There was a problem hiding this comment.
This would also make SolutionStates a sibling of ComMod, which preserves some of the old structure of the code.
There was a problem hiding this comment.
Yes, there are still things left in the ComMod. Ideally, we should gradually take things out of ComMod.
There was a problem hiding this comment.
But would it be better to make SolutionStates a member of Simulation?
There was a problem hiding this comment.
Adding @zasexton to this thread since he's also working on code refactoring and posted a related issue yesterday.
| void set_bc_dir(ComMod& com_mod, Array<double>& lA, Array<double>& lY, Array<double>& lD) | ||
| void set_bc_dir(ComMod& com_mod, SolutionStates& solutions) | ||
| { | ||
| // Local aliases for solution arrays |
There was a problem hiding this comment.
Rename these to be consistent between current and old?
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Current situation
This PR addresses #442 - Encapsulate the Newton iteration in main.cpp.
As discussed in the issue, this is the first step toward implementing a partitioned Fluid-Structure
Interaction (FSI) by creating an
Integratorclass to handle the Newton iteration loop. Thisrefactoring prepares the codebase for future work where separate integrators will be used for fluid
and structure domains.
Release Notes
IntegratorclassIntegratorclass manages solution variables (Ag, Yg, Dg) and handlescomplete Newton iteration workflow
behavior
integrator.handintegrator.cppto CMakeLists.txtDocumentation
integrator.hheader filemain.cppimplementationTesting
fluid/pipe_RCR_3dtest caseCode Coverage: The new
Integratorclass executes the same code paths as the originalimplementation, maintaining existing test coverage. The refactoring does not introduce new physics
or algorithms requiring additional test cases.
Code of Conduct & Contributing Guidelines
Conduct and Contributing
Guidelines.