Skip to content

Conversation

@nijawa
Copy link
Contributor

@nijawa nijawa commented Feb 26, 2024

Changes and Information

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

  • Implemented stochastic version of SIR and SIRS model, with specialized Simulations.
  • Implemented Euler-Maruyama method reusing EulerIntegratorCore and a custom advance function.
  • Time step dt is now updated during integration. This is currently only used by the new SDE models.
  • Protected IntegratorCore for stochastic models to avoid unintended use of ODE integrators.
  • Small fixes: avoid two dangling references, avoid a copy in FlowSimulation.

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

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 #953

nijawa and others added 8 commits January 14, 2024 17:36
New stochastic Versions of advance and simulate

Started implementing an Euler Maruyama Core
expose some members of FlowSimulation for derived classes,
update simulate(_flows) doc comments.
…_flows_stochastic) in a new simulation.h header in the model directory. other small fixes and formatting
@reneSchm reneSchm requested a review from mknaranja March 22, 2024 12:50
@reneSchm
Copy link
Member

I double checked for dangling references. Apparently, gcc 11 and clang 17 either do not care, or more probably fix them silently (dangling references cause undefined behaviour, so the compiler is technically allowed to do anything). However, gcc 13 does complain, and in both dynamic_npis.h and mobility_io.h we used to try and assign a Range r-value to a l-value reference. That is now fixed.

@codecov
Copy link

codecov bot commented Mar 22, 2024

Codecov Report

Attention: Patch coverage is 94.95413% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 96.33%. Comparing base (ce75c58) to head (baa026b).

Files Patch % Lines
cpp/models/sde_sir/simulation.h 86.48% 5 Missing ⚠️
cpp/models/sde_sirs/simulation.h 86.48% 5 Missing ⚠️
cpp/memilio/io/mobility_io.h 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #954      +/-   ##
==========================================
- Coverage   96.36%   96.33%   -0.03%     
==========================================
  Files         123      129       +6     
  Lines        9844    10047     +203     
==========================================
+ Hits         9486     9679     +193     
- Misses        358      368      +10     

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

Copy link
Member

@mknaranja mknaranja left a comment

Choose a reason for hiding this comment

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

Hi @nijawa and @reneSchm. Thank you for this very nice work to finally also include some stochastic equation-based models. Finally with a very nice design. I only have some minor text comments. Tests are still missing but @reneSchm promised to implement them :)

@reneSchm reneSchm requested review from HenrZu and mknaranja March 25, 2024 07:46
Copy link
Member

@mknaranja mknaranja left a comment

Choose a reason for hiding this comment

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

@reneSchm Thanks for the review. With the last minor changes we can then merge :)

this only matters for SDE models, as they use dt in the DerivFunction.
Co-authored-by: Martin J. Kühn <62713180+mknaranja@users.noreply.github.com>
@mknaranja mknaranja merged commit ffa248d into main Mar 27, 2024
@mknaranja mknaranja deleted the stochastic_compartment_playground branch March 27, 2024 11:51
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.

Implement basic stochastic differential equation-based model

5 participants