Skip to content

Support multiple CEP stimuli per domain#586

Merged
michelebucelli merged 4 commits into
SimVascular:mainfrom
samibismar:feature-cep-multiple-stimuli
Jul 14, 2026
Merged

Support multiple CEP stimuli per domain#586
michelebucelli merged 4 commits into
SimVascular:mainfrom
samibismar:feature-cep-multiple-stimuli

Conversation

@samibismar

Copy link
Copy Markdown
Contributor

Current situation

Closes #581.

This PR allows multiple CEP <Stimulus> elements to be defined within the same domain. Each stimulus is parsed, stored, distributed, and evaluated independently, including its own amplitude, timing parameters, and optional spatial bounds.

This allows spatially separated regions within the same CEP domain to be stimulated without creating additional domains solely for stimulus placement.

Existing inputs containing zero or one <Stimulus> preserve their previous behavior.

Release Notes

  • Replaced the single parsed CEP stimulus with a collection of independently owned stimulus parameters.
  • Replaced the single runtime stimType with a collection of runtime stimuli.
  • Added support for repeated <Stimulus> elements under both:
    • an explicit <Domain>
    • the equation-level default domain
  • Each stimulus is evaluated independently using its own timing parameters and spatial bounds.
  • When multiple stimuli are active at the same node and time, their contributions are superimposed by summing their amplitudes.
  • Updated MPI distribution to communicate the number of stimuli and distribute each stimulus independently.
  • Preserved the existing behavior for domains containing zero or one stimulus.

Example:

<Domain id="1">
  ...

  <Stimulus type="Istim">
    <Amplitude> -36.0 </Amplitude>
    <Start_time> 0.0 </Start_time>
    <Duration> 2.0 </Duration>
    <Spatial_bounds>
      <Sphere>
        <Center> 0.0 0.0 3.0 </Center>
        <Radius> 1.5 </Radius>
      </Sphere>
    </Spatial_bounds>
  </Stimulus>

  <Stimulus type="Istim">
    <Amplitude> -36.0 </Amplitude>
    <Start_time> 0.0 </Start_time>
    <Duration> 2.0 </Duration>
    <Spatial_bounds>
      <Sphere>
        <Center> 20.0 7.0 3.0 </Center>
        <Radius> 1.5 </Radius>
      </Sphere>
    </Spatial_bounds>
  </Stimulus>

  ...
</Domain>

Documentation

This feature uses the existing <Stimulus> XML structure and allows it to be repeated within the same CEP domain. No new stimulus parameters are introduced.

A follow-up update will be needed in the svMultiPhysics documentation.

Testing

Successfully compiled on macOS.

Validated using AP Niederer CEP benchmark cases covering:

  • no stimulus
  • one stimulus
  • a zero-amplitude stimulus
  • two stimuli whose amplitudes sum to the equivalent single-stimulus value
  • one active stimulus together with an inactive stimulus
  • overlapping stimuli with additive amplitudes
  • two disjoint spherical stimulus regions within the same domain

All expected comparisons passed in serial and with MPI using 2 processes.

A visual validation was also run with 4 MPI processes using two spatially separated stimuli within the same domain. The results showed two distinct initial activation sites followed by propagation from both regions.

Code of Conduct & Contributing Guidelines

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@michelebucelli michelebucelli left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @samibismar! I have only one small comment below but otherwise this looks good.

Beyond that, I think this PR could be a good time to make sure that the new features (introduced here and in #579) are covered by the integration tests. I think it would be good to do it without introducing new tests, if possible - do you know if any of the cep tests have multiple stimuli, currently applied with multiple domains?

Comment thread Code/Source/solver/Parameters.h
@codecov

codecov Bot commented Jul 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 82.60870% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.46%. Comparing base (0337748) to head (aaa460c).

Files with missing lines Patch % Lines
Code/Source/solver/Parameters.cpp 33.33% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #586      +/-   ##
==========================================
+ Coverage   72.33%   72.46%   +0.12%     
==========================================
  Files         241      241              
  Lines       38632    38646      +14     
  Branches     6641     6644       +3     
==========================================
+ Hits        27944    28004      +60     
+ Misses      10453    10407      -46     
  Partials      235      235              

☔ View full report in Codecov by Harness.
📢 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.

@samibismar

Copy link
Copy Markdown
Contributor Author

Thank you @michelebucelli, I replied to your inline comment about the use of std::unique_ptr with my reasoning behind that choice.

I found that spiral_BO_2d is the existing CEP case that uses two separate domains for two stimulus sites. Domains 2 and 3 use the same electrophysiology model, conductivity, and ODE solver, so I think it may be a good candidate for covering both this PR and #579.

My thought is to merge the current Domain 3 region into Domain 2 by relabeling the domain IDs, remove the separate <Domain id="3"> block, and place both stimuli inside Domain 2 with spatial bounds matching their original regions. This would preserve the original two-site setup while representing it using multiple bounded stimuli in one domain, which seems closer to the intended use of the new features.

The current integration test only runs one time step, so the second stimulus at t = 440 would still be parsed and distributed but would not become active during the test. Would you prefer to preserve the existing timing and reference results, or adjust the test so that both stimuli activate during the integration test?

Does this sound like the approach you had in mind?

@michelebucelli

Copy link
Copy Markdown
Collaborator

@samibismar I think your suggestion makes sense. I would leave the test setting exactly as it is, even if the second stimulus doesn't fire. The code paths for multiple stimuli will still be used to some extent.

What I would do is the following:

  1. run the test as it is currently implemented for a relatively long time span (not just one time step) and store its solution for reference;
  2. change the test to use the new framework for multiple stimuli;
  3. run the test again and verify consistency with the old solution (ideally, they should bit-by-bit identical, as long as the stimulus domains can be represented exactly by the new spatial bounds);
  4. if they are, restore the short final time;
  5. check that the test passes when run through pytest (which is to say, check that the test matches the reference solution within tolerance).

@samibismar

Copy link
Copy Markdown
Contributor Author

Thanks @michelebucelli, that makes sense. I’ll generate a longer-run solution with the current test setup, convert the case to use two spatially bounded stimuli within one domain, and compare the long-run solutions. I’ll then restore the original short test duration and verify it through pytest. I’ll let you know what results I get once I’m done.

@ktbolt

ktbolt commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

@michelebucelli We do need to think about refactoring Parameters I think; it is a bit of a monster.

@samibismar

Copy link
Copy Markdown
Contributor Author

@michelebucelli I updated spiral_BO_2d to use two spatially bounded stimuli within Domain 2 and merged the former Domain 3 cells into Domain 2.

I ran the original and modified cases for 100 time steps and compared the outputs every 10 steps. The mesh and all saved solution data, including Membrane_potential and Calcium, were bitwise identical at all ten checkpoints. I excluded Domain_ID because its change from Domain 3 to Domain 2 is intentional.

I then restored the original one-step test duration and verified the case through pytest; it passes against the existing reference with 1, 3, and 4 MPI processes.

@michelebucelli michelebucelli left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @samibismar! The updated test looks good to me.

@michelebucelli

Copy link
Copy Markdown
Collaborator

@ktbolt

We do need to think about refactoring Parameters I think; it is a bit of a monster.

I agree! I have some half-formed thoughts about this, stemming from other refactoring work (most notably #540 and #578). If you want, I can open an issue about it so we can start discussing this more concretely. I'd love to hear your and @zasexton's thoughts about it (and I don't know if @zasexton's planned refactoring also includes parameters or not).

@zasexton

Copy link
Copy Markdown
Collaborator

I'd be interested to hear your thoughts @michelebucelli . I won't be immediately touching the Parameters.cpp file except for occasional wiring of the lower-level finite element technology options (as we agree on design considerations in these refactoring components).

@ktbolt

ktbolt commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

@michelebucelli Your half-formed thoughts are welcome, perhaps superior to my fully-formed thoughts ...

At the least the classes in Parameters.h,cpp need to be broken out into separate files into some sort of io directory.

Please open an Issue for this.

@ktbolt ktbolt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good !

@michelebucelli
michelebucelli merged commit 9b5ed80 into SimVascular:main Jul 14, 2026
6 checks passed
@samibismar
samibismar deleted the feature-cep-multiple-stimuli branch July 14, 2026 22:42
@samibismar

Copy link
Copy Markdown
Contributor Author

@ktbolt, should I open a follow-up issue in SimVascular/simvascular.github.io to document the multiple-stimulus capability?

@ktbolt

ktbolt commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

@samibismar Please update the SimVascular/simvascular.github.io document for the multiple-stimulus capability and be sure to make clear what any default behavior might be.

@samibismar

Copy link
Copy Markdown
Contributor Author

@ktbolt, I opened SimVascular/simvascular.github.io#102 with the documentation update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support multiple CEP stimuli per domain

4 participants