SimVascular / svZeroDSolver

A C++ lumped-parameter solver for blood flow and pressure in hemodynamic networks
Other
6 stars 18 forks source link

Simplify creation of new blocks #71

Closed mrp089 closed 9 months ago

mrp089 commented 10 months ago

Currently, we have lots of block-specific code in SimulationParameters, which builds the 0D model in load_simulation_model. When adding a new block, you have to modify this function in several places in addition to creating your new block (#66.)

Ideally, everything block-specific should be in the blocks themselves, which SimulationParameters should access. An example is reading input parameters (e.g., here for RCR):

https://github.com/StanfordCBCL/svZeroDPlus/blob/3659f6ddee9ffa69ad1d708bd049f4c95b5cae22/src/solve/SimulationParameters.cpp#L264-L267

I thus propose the following:

  1. Move all model parameters to the blocks
  2. Create a central "block registry" with the following groups:
    • vessel (blood_vessel)
    • junction (junction, resistive_junction, blood_vessel_junction)
    • boundary condition (windkessel_bc, closed_loop_rcr_bc, flow_bc, resistnce_bc, pressure_bc, open_loop_coronary_bc, closed_loop_coronary_lefT_bc, closed_loop_coronary_right_bc)
    • closed loop (closed_loop_heart_pulmonary)
  3. Replace switch/case statement in Model::add_block with block registry

I think this would simplify adding new blocks and shorten SimulationParameters.cpp. When creating a new block, you'd only have to touch two places: the new block and the block registry. @ktbolt, @menon-karthik, does this make sense?

ktbolt commented 10 months ago

@mrp089 You have read my mind, was thinking about this exact thing!

The load_simulation_model function is a big hairy monster that needs to be broken up into a bunch of smaller functions.

The block registry thing seems like a nice and simple implementation of a vessel factory.

It would be good to replace all of the parameter names literals (e.g., model.add_parameter(bc_values["Rp"]).

Another confusing thing (to me anyway) is the use of vessel class enums (e.g. ClosedLoopHeartPulmonary::ParamId) to identify the location of a parameter, maybe this is the only to do this.

mrp089 commented 10 months ago

@ktbolt, perfect! Do you want to do the rearrangement or should I have a try at it? I think I know how to move the block-specific code, but I think I lack the C++ skills to come up with a block factory.

I think the reason for the enums in ClosedLoopHeartPulmonary::ParamId is that it's more reliable to code up all the equations using parameter names rather than indices.

ktbolt commented 10 months ago

@mrp089 Have a try at the rearrangement, move model parameters to the blocks and such.

The Model::add_block() method is the factory although we might want to use a map of lamba functions instead instead of a switch statement.

ktbolt commented 9 months ago

@mrp089 It is s good idea to provide a high-level description about what was actually done. This provides a helpful context when reviewing code in a pull request.