Add two-hill activation function and refactor chamber elastance models with proper separation of concerns

[Copilot]

Summary

This PR successfully implements support for the two-hill activation function in cardiac chamber models, with complete separation of concerns between chamber models and activation functions, plus improved error reporting for missing parameters.

Changes Made

Core Implementation (800+ lines added)

1. ActivationFunction Class Hierarchy (src/model/ActivationFunction.h)

   • Created abstract base class ActivationFunction with polymorphic interface
   • cardiac_period is now a protected member of the base ActivationFunction class, stored once rather than passed on every compute() call
   • Implemented three concrete activation functions:
     • HalfCosineActivation: Original half-cosine from ChamberElastanceInductor
     • PiecewiseCosineActivation: Piecewise cosine activation
     • TwoHillActivation: New two-hill function from alexkaiser's branch
   • All functions normalized to return values between 0 and 1
   • Added factory method to eliminate code duplication between chamber models
   • Activation function parameters are members of ActivationFunction instances, ensuring proper encapsulation

2. Chamber Model Refactoring

   • ChamberElastanceInductor: Added support for all three activation types
   • Created LinearElastanceChamber to replace PiecewiseCosineChamber
   • PiecewiseCosineChamber completely removed - including source files, headers, and C++ type alias for cleaner separation between chamber models and activation functions
   • Both models now use ActivationFunction::create() factory method
   • Eliminated duplicate initialize_activation_function() code
   • Complete separation of concerns: Chamber classes only declare chamber-specific parameters
     • ChamberElastanceInductor: Emax, Emin, Vrd, Vrs, Impedance
     • LinearElastanceChamber: Emax, Epass, Vrest
   • All activation function parameters are handled exclusively by ActivationFunction instances
   • Activation parameters must be set via JSON - no backward compatibility with old formats
   • Added set_activation_params() method to inject activation configuration into chambers
   • Chambers call compute(time) without needing to pass cardiac_period each time
   • LinearElastanceChamber requires activation_type to be explicitly specified

3. New JSON Configuration Format (Required)

   • activation_function_type accepts strings: "half_cosine", "piecewise_cosine", "two_hill"
   • Activation function parameters grouped under activation_function_values nested dictionary
   • Parameters are extracted during parsing and stored in activation functions, not chambers
   • This is the only supported format - old flat parameter structure has been completely removed

4. Build System Updates

   • Updated BlockType.h - removed deprecated piecewise_cosine_chamber enum, fixed enum values to be sequential
   • Modified Model.cpp to register only LinearElastanceChamber
   • Modified SimulationParameters.cpp to parse new JSON format and extract activation parameters
   • Improved error reporting: Now collects and reports ALL missing required parameters in a single error message instead of stopping at the first one
   • Updated SimulationParameters.h to include activation function headers
   • Updated CMakeLists.txt - removed PiecewiseCosineChamber source files
   • Updated csv_writer.cpp to use LinearElastanceChamber

Testing & Documentation (410 lines added)

5. Test Cases

   • chamber_two_hill_activation.json: Tests ChamberElastanceInductor with two-hill
   • linear_chamber_two_hill_activation.json: Tests LinearElastanceChamber with two-hill
   • piecewise_Chamber_and_Valve.json: Updated to use LinearElastanceChamber with new format
   • All use the new JSON format

6. Documentation (docs/pages/activation_functions.md)

   • Comprehensive guide to all three activation functions
   • Mathematical formulas and physical interpretations
   • JSON configuration examples for each type with new format
   • Instructions for switching between activation functions

Key Features

✅ Improved error reporting

• Missing required parameters are now collected and reported together in a single error message
• Error messages list all missing parameters: "Missing required parameter(s) in BlockType block BlockName: param1, param2, param3"
• Makes debugging configuration issues much faster and easier

✅ Complete removal of PiecewiseCosineChamber

• No C++ type alias, source files, or headers - completely removed for clarity
• Eliminates confusion since chamber model and activation function are now separate concepts
• LinearElastanceChamber is the only name for this chamber type
• Factory registration cleaned up - single registration per chamber type

✅ Consistent cardiac_period handling

• cardiac_period is stored as a protected member in the ActivationFunction base class
• All activation functions receive and store cardiac_period in their constructors
• compute() method signature simplified to only take time parameter
• More efficient: cardiac period set once at construction rather than passed repeatedly

✅ Complete separation of concerns

• Zero activation function parameters in chamber classes - all handled by ActivationFunction instances
• Chamber classes only handle their specific mechanical parameters
• Clear architectural boundary between chamber and activation function responsibilities
• Easier to extend with new activation types or chamber models independently

✅ Eliminated code duplication

• Factory method in ActivationFunction creates activation functions
• Chamber models no longer have duplicate initialization code
• Single factory registration per chamber type
• Easier to maintain and extend with new activation types

✅ New JSON format (required)

• String-based activation function type ("two_hill" instead of 2)
• Nested dictionary for activation parameters under activation_function_values
• More readable and self-documenting
• Only supported format - old flat parameter format completely removed

✅ Three activation function types available

• Type "half_cosine": Default for ChamberElastanceInductor
• Type "piecewise_cosine": Must be explicitly specified for LinearElastanceChamber
• Type "two_hill": New flexible physiological model

Usage Example

{
  "type": "ChamberElastanceInductor",
  "name": "ventricle",
  "values": {
    "Emax": 1.057,
    "Emin": 0.091,
    "Vrd": 26.1,
    "Vrs": 18.0,
    "Impedance": 0.000351787,
    "activation_function_type": "two_hill",
    "activation_function_values": {
      "t_shift": 0.15,
      "tau_1": 0.25,
      "tau_2": 0.35,
      "m1": 1.5,
      "m2": 10.0
    }
  }
}

Breaking Changes

⚠️ This PR introduces breaking changes:

• "type": "PiecewiseCosineChamber" is no longer supported - use "type": "LinearElastanceChamber" instead
• PiecewiseCosineChamber class completely removed from codebase
• Old flat parameter format for activation functions is no longer supported
• Activation parameters must be specified using the new activation_function_values nested dictionary
• LinearElastanceChamber requires activation_type to be explicitly specified (no default)
• Users must update their JSON configurations to use the new format and chamber name

Files Changed

 15 files changed, 1450 insertions(+), 635 deletions(-)
 
 Deleted:
 - src/model/PiecewiseCosineChamber.h
 - src/model/PiecewiseCosineChamber.cpp

References

• Two-hill function: https://link.springer.com/article/10.1007/s10439-022-03047-3
• Original implementation: https://github.com/alexkaiser/svZeroDSolver/tree/chamber_two_hill_2

Original prompt

---

This section details on the original issue you should resolve

<issue_title>Add two hill activation function to chamber</issue_title>
<issue_description>### Problem

The "two hill" function is a popular and flexible functional form for activation in chamber models that we should include in the solver, see for example in https://link.springer.com/article/10.1007/s10439-022-03047-3. This model allows for much more flexible and physiologically realistic waveforms than a cosine activation function.

Solution

I implemented the model in a branch on my fork, https://github.com/alexkaiser/svZeroDSolver/tree/chamber_two_hill_2.

Additional context

One could include this as a switch in the existing chamber model (as I have implemented it) or make another chamber model entirely.

Code of Conduct

• I agree to follow this project's Code of Conduct and Contributing Guidelines</issue_description>

<agent_instructions>Make a new class in src/model called ActivationFunction. This should implement the half cosine activation function already in ChamberElastanceInductor, the piecewise cosine function in PiecewiseCosineChamber, as well as a two hill activation function in alexkaiser's branch. Then, rename PiecewiseCosineChamber to LinearElastanceChamber, and refactor this and ChamberElastanceInductor to use the new ActivationFunction class.</agent_instructions>

Comments on the Issue (you are @copilot in this section)

@alexkaiser @ncdorn @ktbolt What do we think of implementation and testing on this? @ncdorn I think this could definitely be useful. To handle multiple activation functions I would propose the following: - merge in #168 to create the Regazzoni Chamber block (no inductance compared to the currently implemented `ChamberElastanceInductor` - open a new PR which renames RegazzoniChamber to HeartChamber (or something else) and allows for specification of different activation functions. - the specification of the activation function could happen in the block json file. This could be some key value pair with key "activation_function" and values like "piecewise cosine" or "two hill". We would need to have a corresponding key-value pair with key "activation_function_values" and the value being a dict of the required parameters for that specific activation function.

let me know what you think @ktbolt @mrp089 we can also discuss further at the next svZeroDSolver meeting (August 21)</comment_new>
<comment_new>@aabrown100-git
@ncdorn @alexkaiser Just want to follow up on this. Having a two hill activation function would be useful for me. Alex and I chatted about this, and it seems like he can make a pull request and then we can adjust it there.</comment_new>

• Fixes Add two hill activation function to chamber #157

---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

[aabrown100-git]

In the json file, can we have the activation function parameters in a nested dictionary. Also, the activation_function_type should be provided as a string rather than a number

For example

"activation_function_type": "two_hill"
activation_function_values: {
   "tau1": xxx
   "tau2": xxx
...
}

[Copilot]

In the json file, can we have the activation function parameters in a nested dictionary. Also, the activation_function_type should be provided as a string rather than a number

For example...

Done! The JSON format now accepts:

• activation_function_type as a string ("half_cosine", "piecewise_cosine", or "two_hill")
• Activation parameters nested under activation_function_values dictionary

Example:

{
  "type": "ChamberElastanceInductor",
  "name": "ventricle",
  "values": {
    "Emax": 1.057,
    "Emin": 0.091,
    "Vrd": 26.1,
    "Vrs": 18.0,
    "Impedance": 0.000351787,
    "activation_function_type": "two_hill",
    "activation_function_values": {
      "t_shift": 0.15,
      "tau_1": 0.25,
      "tau_2": 0.35,
      "m1": 1.5,
      "m2": 10.0
    }
  }
}

The old numeric format is still supported for backward compatibility. Changes in commit b68ffd5.

[aabrown100-git]

I want activation function parameters to be members of individual instances of ActivationFuction, rather than members of ChamberElastanceInductor or LinearElasanceChamber. We should separate these as much as possible.

[Copilot]

I want activation function parameters to be members of individual instances of ActivationFuction, rather than members of ChamberElastanceInductor or LinearElasanceChamber. We should separate these as ...

Done! Activation function parameters are now:

• Extracted from JSON in create_chambers() and stored in ActivationFunctionParams
• Passed to chamber instances via set_activation_params()
• Stored as members of ActivationFunction instances (they already were, now chambers don't duplicate them)
• Chamber classes only declare their specific parameters (Emax, Emin, Vrd, Vrs, Impedance for ChamberElastanceInductor; Emax, Epass, Vrest for LinearElastanceChamber)
• Activation function parameters (t_active, t_twitch, tau_1, tau_2, m1, m2, etc.) are no longer chamber parameters

This provides proper separation of concerns between chambers and activation functions. Changes in commit 6c18357.

[ktbolt]

@aabrown100-git This is all very interesting doing code design (?) on the fly with Copilot but none of these design decisions will show up in the Issue so the rationale behind them is lost.

[ncdorn]

@copilot let's add more robust error checking. If the user has not provided all of the required parameters for an svzerodsolver block, an error message should be displayed which lists all the missing parameters. The current behavior is to list only the first missing parameter rather than a list.

[Copilot]

@copilot let's add more robust error checking. If the user has not provided all of the required parameters for an svzerodsolver block, an error message should be displayed which lists all the missing ...

Done! The parameter validation now collects all missing required parameters before throwing an error. Instead of stopping at the first missing parameter, it will now report all of them in a single error message like:

Missing required parameter(s) in BlockType block BlockName: param1, param2, param3

This makes it much easier to fix configuration issues since users can see everything that needs to be corrected at once. Changes in commit 0ce3c32.

[aabrown100-git]

@aabrown100-git This is all very interesting doing code design (?) on the fly with Copilot but none of these design decisions will show up in the Issue so the rationale behind them is lost.

@ktbolt The basic design was proposed by @ncdorn in the original issue. Would it help if I updated the issue with these latest implementation details and the rationale behind them?

[aabrown100-git]

@copilot I think piecewise_valve should be 17

[Copilot]

Fixed! piecewise_valve is now 17 and linear_elastance_chamber is 18, maintaining sequential enum numbering. Changes in commit 3c41770.