Skip to content

1101 Extend integrator classes and reimplement SDE models. #1296

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

1101 Extend integrator classes and reimplement SDE models. #1296

wants to merge 11 commits into from

Conversation

reneSchm
Copy link
Member

@reneSchm reneSchm commented Jun 10, 2025 •
edited
Loading

Changes and Information

Please briefly list the changes (main added features, changed items, or corrected bugs) made:

  • Extend IntegratorCore and OdeIntegrator to accept a fixed number of DerivFunctions (rather than only one).
  • Add classes SimulationBase and FlowSimulationBase to avoid code duplication between different Simulations.
  • Add class StochasticModel which is a Compartmental- or FlowModel with additional get_noise function.
  • Add class StochasticSimulation, which makes use of a second DerivFunction to evaluate get_noise.
  • Add class EulerMaruyamaIntegratorCore, implementing a (step of a) Euler-Maruyama scheme.

If need be, add additional information and what the reviewer should look out for in particular:

  • Check the naming of new classes, in particular the new Order template parameter, the get_noise function and the XdeIntegrator (maybe "DeIntegrator" or just "Integrator"?).
  • Are there Simulation classes that should be rebuild on the new *base classes now?
  • Should the rescaler used by Euler-Maruyama be modular?

Merge Request - Guideline Checklist

Please check our git workflow. Use the draft feature if the Pull Request is not yet ready to review.

Checks by code author

  • Every addressed issue is linked (use the "Closes #ISSUE" keyword below)
  • New code adheres to coding guidelines
  • No large data files have been added (files should in sum not exceed 100 KB, avoid PDFs, Word docs, etc.)
  • Tests are added for new functionality and a local test run was successful (with and without OpenMP)
  • Appropriate documentation for new functionality has been added (Doxygen in the code and Markdown files if necessary)
  • Proper attention to licenses, especially no new third-party software with conflicting license has been added
  • (For ABM development) Checked benchmark results and ran and posted a local test above from before and after development to ensure performance is monitored.

Checks by code reviewer(s)

  • Corresponding issue(s) is/are linked and addressed
  • Code is clean of development artifacts (no deactivated or commented code lines, no debugging printouts, etc.)
  • Appropriate unit tests have been added, CI passes, code coverage and performance is acceptable (did not decrease)
  • No large data files added in the whole history of commits(files should in sum not exceed 100 KB, avoid PDFs, Word docs, etc.)
  • On merge, add 2-5 lines with the changes (main added features, changed items, or corrected bugs) to the merge-commit-message. This can be taken from the briefly-list-the-changes above (best case) or the separate commit messages (worst case).

Closes #1009
Closes #1101
Closes #1165
Closes #1218

@reneSchm reneSchm self-assigned this Jun 10, 2025
@codecov Codecov
Copy link

codecov bot commented Jun 11, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 98.14815% with 3 lines in your changes missing coverage. Please review.

Project coverage is 97.29%. Comparing base (7f73862) to head (c49b895).

Files with missing lines Patch % Lines
cpp/memilio/compartments/stochastic_model.h 0.00% 2 Missing ⚠️
cpp/memilio/compartments/stochastic_simulation.h 94.73% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1296      +/-   ##
==========================================
+ Coverage   97.21%   97.29%   +0.08%     
==========================================
  Files         167      169       +2     
  Lines       14960    14851     -109     
==========================================
- Hits        14543    14449      -94     
+ Misses        417      402      -15

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

@reneSchm reneSchm requested a review from HenrZu June 16, 2025 08:19
@reneSchm
Copy link
Member Author

@nijawa Could you double check that this PR solves the NaN issues (mathematically) correctly?

HenrZu
HenrZu requested changes Jun 17, 2025
View reviewed changes
Copy link
Contributor

@HenrZu HenrZu left a comment

Choose a reason for hiding this comment

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

code is great :) Some comments. Some functions are short in/missing documentation. Also consider the readthedocs doc. However, it would make sense to update this in the branch there.

cpp/memilio/math/integrator.h
using DerivFunction =
std::function<void(Eigen::Ref<const Eigen::VectorX<FP>> y, FP t, Eigen::Ref<Eigen::VectorX<FP>> dydt)>;

template <typename FP = double>
template <typename FP, size_t Order>
Copy link
Contributor

Choose a reason for hiding this comment

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

the name "Order" could be misleading (confusing with with integration order)

Copy link
Member Author

Choose a reason for hiding this comment

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

I know, but I have not come up with a better name. Maybe just NumDerivs or simply N works well enough? I will add a "@tparam" description later anyways

cpp/memilio/math/euler_maruyama.h
return true;
}

void rescale(Eigen::Ref<Eigen::VectorX<FP>> x) const
Copy link
Contributor

Choose a reason for hiding this comment

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

doc

cpp/memilio/compartments/stochastic_model.h
* See the License for the specific language governing permissions and
* limitations under the License.
*/
#ifndef MIO_COMPARTMENTS_STOCHASTIC_MODEL_H
Copy link
Contributor

Choose a reason for hiding this comment

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

can you also adjust the header guards form the other file here with the new notation (especially, flow model)

Copy link
Member Author

Choose a reason for hiding this comment

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

should I also rename compartmentalmodel.h to compartmental_model.h?

cpp/memilio/compartments/stochastic_model.h
FlowModel<FP, Comp, Pop, Params, Flows>>;
using Base::Base;

virtual void get_noise(Eigen::Ref<const Eigen::VectorX<FP>> /*pop*/, Eigen::Ref<const Eigen::VectorX<FP>> /*y*/,
Copy link
Contributor

Choose a reason for hiding this comment

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

doc

@github-project-automation github-project-automation bot moved this from 🏗 Development to 👀 Review in MEmilio: Equation-Based-Models Development Jun 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HenrZu HenrZu HenrZu requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@reneSchm reneSchm

Labels
None yet
Projects
MEmilio: Equation-Based-Models Development
Status: 👀 Review
Milestone
No milestone
2 participants
@reneSchm @HenrZu