Encapsulate Newton iteration in Integrator class (#442) #450

Eleven7825 · 2025-10-13T02:36:25Z

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 Integrator class to handle the Newton iteration loop. This
refactoring prepares the codebase for future work where separate integrators will be used for fluid
and structure domains.

Release Notes

Refactoring: Encapsulated Newton iteration logic into a new Integrator class
Architecture: New Integrator class manages solution variables (Ag, Yg, Dg) and handles
complete Newton iteration workflow
Backward Compatibility: No changes to physics or numerics - pure refactoring with identical
behavior
Build System: Added integrator.h and integrator.cpp to CMakeLists.txt

Documentation

Added comprehensive class documentation in integrator.h header file
Documented all public and private methods with parameter descriptions
Added inline comments explaining the Newton iteration workflow
Maintained existing code comments from original main.cpp implementation
Updated file headers with proper copyright notices

Testing

Build: Compiles successfully without errors or warnings
Manual Testing: Validated with fluid/pipe_RCR_3d test case
- 2 timesteps completed successfully
- Newton iterations converge correctly (residuals: ~10^-13)
- Output matches expected convergence behavior
Integration: All existing functionality preserved

Code Coverage: The new Integrator class executes the same code paths as the original
implementation, maintaining existing test coverage. The refactoring does not introduce new physics
or algorithms requiring additional test cases.

Code of Conduct & Contributing Guidelines

I agree to follow the Code of
Conduct and Contributing
Guidelines.

- 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 · 2025-10-13T17:08:03Z

Codecov Report

❌ Patch coverage is 81.78138% with 90 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.58%. Comparing base (3e2e2ba) to head (63e5753).

Files with missing lines	Patch %	Lines
Code/Source/solver/initialize.cpp	40.00%	24 Missing ⚠️
Code/Source/solver/ris.cpp	31.57%	13 Missing ⚠️
Code/Source/solver/read_msh.cpp	0.00%	12 Missing ⚠️
Code/Source/solver/post.cpp	0.00%	7 Missing ⚠️
Code/Source/solver/set_bc.cpp	85.10%	7 Missing ⚠️
Code/Source/solver/main.cpp	76.00%	6 Missing ⚠️
Code/Source/solver/txt.cpp	64.70%	6 Missing ⚠️
Code/Source/solver/Integrator.cpp	97.78%	5 Missing ⚠️
Code/Source/solver/nn.cpp	58.33%	5 Missing ⚠️
Code/Source/solver/all_fun.cpp	89.47%	2 Missing ⚠️
... and 3 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #450      +/-   ##
==========================================
+ Coverage   67.52%   67.58%   +0.05%     
==========================================
  Files         169      170       +1     
  Lines       32671    32721      +50     
  Branches     5728     5734       +6     
==========================================
+ Hits        22062    22113      +51     
+ Misses      10472    10471       -1     
  Partials      137      137

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mrp089

Thanks for taking a crack at this, @Eleven7825! I've added some comments.

mrp089 · 2025-10-14T22:13:11Z

.gitignore

Are all those .gitignore's necessary?

Code/Source/solver/Integrator.cpp

Code/Source/solver/integrator.cpp

Code/Source/solver/Integrator.cpp

mrp089 · 2025-10-14T22:19:50Z

Code/Source/solver/Integrator.cpp

+      #ifdef debug_integrator_step
+      dmsg << "Set coupled BCs " << std::endl;
+      #endif
+      set_bc::set_bc_cpl(com_mod, cm_mod);


Wherever feasible, add the debug outputs to the individual functions to de-clutter step()

Code/Source/solver/main.cpp

mrp089 · 2025-10-14T22:27:45Z

Code/Source/solver/integrator.cpp

+  #endif
+
+  // Inner loop for Newton iteration
+  inner_count_ = 1;


Be consistent in comments and variable names between Newton and inner iterations. We should pick one and stick to it. I prefer Newton vs. time loop over outer vs. inner loop because it's a bit clearer.

ktbolt · 2025-10-15T01:20:23Z

@Eleven7825 This is a big change and needs to be discussed with the solver team.

mrp089 · 2025-10-15T20:00:12Z

Sounds good! Who do we talk to and when?

Eleven7825 · 2025-10-15T20:06:41Z

Maybe we could bring this up this Friday during the refactory meeting?

ktbolt · 2025-10-15T20:45:14Z

@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 svMultiPhysics c++ OO refactor planning meeting.

Code/Source/solver/CMakeLists.txt

mrp089 · 2025-10-20T20:43:48Z

@dcodoni suggested removing com_mod from all function calls in Integrator (which will require changes in other functions/classes, too). This will require following the function calls with that have com_mod as an argument all the way down and figuring out which parts of com_mod are actually needed. It might be necessary to create a few structs in Integrator that encapsulate a few items to avoid having functions with a ton of arguments.

- 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.

@brief

- 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.

Eleven7825 · 2025-10-24T20:02:00Z

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

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.cpp there are a few things that are not adapted yet to the n's, potentially set_bc::set_bc_dir and txt_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.

mrp089 · 2025-11-07T20:09:50Z

Code/Source/solver/nn.cpp

          for (int i = 0; i < lX.nrows(); i++) {
            // Add displacement at timestep n+1
-            lX(i,a) = lX(i,a) + com_mod.Dn(i,Ac);
+            if (Dn != nullptr) {


Have exactly the same behavior as before: only use Dn

mrp089 · 2025-11-07T20:10:31Z

Code/Source/solver/ComMod.h

-
-    /// @brief New integrated variables (displacement)
-    Array<double>  Dn;
+    /// @brief Ao, Do, Yo moved to Integrator class (complete ownership transfer)


Remove comment: history is already preserved in git, no need to have in the code

mrp089 · 2025-11-07T20:11:18Z

Code/Source/solver/Integrator.cpp

-  auto& Dn = com_mod.Dn;
+  auto& An = An_;  // Use member variable
+  auto& Yn = Yn_;
+  auto& Dn = Dn_;


Why do we need to define these three?

mrp089 · 2025-11-07T20:11:39Z

Code/Source/solver/Integrator.cpp

  dmsg << "Apply boundary conditions ..." << std::endl;
  #endif

+  auto& Yn = Yn_;


Why is this needed?

mrp089 · 2025-11-07T20:12:18Z

Code/Source/solver/Integrator.cpp

+///   Yg - velocity
+///   Dg - displacement
+///
+void Integrator::pici(Array<double>& Ag, Array<double>& Yg, Array<double>& Dg)


pici -> initiator

mrp089 · 2025-11-07T20:27:12Z

Code/Source/solver/ris.cpp

  const int cEq = com_mod.cEq;

-  auto& An = com_mod.An;
+  // An, Dn, and Yn are now passed as parameters


Remove comment

mrp089 · 2025-11-07T20:27:46Z

Code/Source/solver/ris.cpp

-void ris_updater(ComMod& com_mod, CmMod& cm_mod)
+/// @brief  This subroutine updates the resistance and activation flag for the
+/// closed and open configurations of the RIS surfaces
+void ris_updater(ComMod& com_mod, CmMod& cm_mod, Array<double>& An, Array<double>& Dn, Array<double>& Yn, Array<double>& Ao, Array<double>& Do, Array<double>& Yo)


Keep consistent order everywhere

mrp089 · 2025-11-07T20:28:16Z

Code/Source/solver/ris.cpp

  const int cEq = com_mod.cEq;

-  auto& An = com_mod.An;
+  // An, Dn, and Yn are now passed as parameters


remove comment

mrp089 · 2025-11-07T20:29:42Z

Code/Source/solver/uris.cpp

  // auto& Ad = com_mod.Ad;
  // auto& Dn = com_mod.Dn;
-  auto& Yn = com_mod.Yn;
+  // Yn is now passed as a parameter


Remove comment

mrp089 · 2025-11-07T20:30:01Z

Code/Source/solver/uris.cpp

  // auto& Ad = com_mod.Ad;
  // auto& Dn = com_mod.Dn;
-  auto& Yn = com_mod.Yn;
+  // Yn is now passed as a parameter


Remove comment

…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).

mrp089 · 2025-11-13T21:01:55Z

Six down, many to go! @Eleven7825 successfully removed the old and new solutions from ComMod.

We now explicitly pass Ao, An, Do, Dn, Yo, Yn through all the functions. At times, it can be confusing to keep track of the numbering of function arguments when more than one vector is passed. My idea was, as a final step of this PR, to encapsulate Ao, An, Do, Dn, Yo, Yn in a Solution class that we pass. @ktbolt, @dcodoni, please let me know what you think.

Eleven7825 added 4 commits October 13, 2025 01:57

Fix: Add missing iEqOld variable for output calls

7eec430

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.

mrp089 requested changes Oct 14, 2025

View reviewed changes

mrp089 added 2 commits October 17, 2025 11:34

Merge branch 'main' into feature/issue-442-integrator-class

d095026

change license header

1d7c5cf

ktbolt self-requested a review October 18, 2025 02:02

ktbolt reviewed Oct 18, 2025

View reviewed changes

Code/Source/solver/CMakeLists.txt Outdated Show resolved Hide resolved

Eleven7825 added 5 commits October 23, 2025 03:31

Eleven7825 marked this pull request as ready for review October 23, 2025 18:58

Eleven7825 added 4 commits November 6, 2025 17:14

Merge remote changes with Ao, Do, Yo extraction work

bbb88d6

mrp089 requested changes Nov 7, 2025

View reviewed changes

Eleven7825 added 2 commits November 12, 2025 23:50

Merge branch 'main' into feature/issue-442-integrator-class

63e5753

Encapsulate Newton iteration in Integrator class (#442) #450

Are you sure you want to change the base?

Encapsulate Newton iteration in Integrator class (#442) #450

Uh oh!

Conversation

Eleven7825 commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current situation

Release Notes

Documentation

Testing

Code of Conduct & Contributing Guidelines

Uh oh!

codecov bot commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mrp089 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktbolt commented Oct 15, 2025

Uh oh!

mrp089 commented Oct 15, 2025

Uh oh!

Eleven7825 commented Oct 15, 2025

Uh oh!

ktbolt commented Oct 15, 2025

Uh oh!

Uh oh!

mrp089 commented Oct 20, 2025

Uh oh!

Eleven7825 commented Oct 24, 2025

Uh oh!

mrp089 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrp089 commented Nov 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Eleven7825 commented Oct 13, 2025 •

edited

Loading

codecov bot commented Oct 13, 2025 •

edited

Loading