Skip to content

Conversation

kilianvolmer
Copy link
Contributor

@kilianvolmer kilianvolmer commented Oct 1, 2025

Changes and Information

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

  • The SMM Model now works with an arbitrary number of additional AgeGroups
  • Influences can come from different regions

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

  • The code should be backwards compatible to the current implementation
  • The simulation algorithm itself was not changed, the only difference there is in the function that calculates the current rates.

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 within the code (Doxygen) for new functionality has been added in the code
  • Appropriate external documentation (ReadTheDocs) for new functionality has been added to the online documentation
  • 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 #1341

Copy link

codecov bot commented Oct 2, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.28%. Comparing base (fbaf0eb) to head (17cd0b9).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1388      +/-   ##
==========================================
- Coverage   97.28%   97.28%   -0.01%     
==========================================
  Files         175      175              
  Lines       15306    15342      +36     
==========================================
+ Hits        14891    14925      +34     
- Misses        415      417       +2     

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

@kilianvolmer kilianvolmer requested a review from jubicker October 2, 2025 11:26
@reneSchm
Copy link
Member

reneSchm commented Oct 2, 2025

Two comments on this:

Fixing the copyright notice is entirely out of scope of this PR. That would be fine, if the changes was limited to a file with other modifications, which is not the case. Please move the copyright changes to a new PR.

I guess gluing on the group indices works, but having to use std::apply whenever we need to index anything is frankly an awful user experience. Can you find a different solution?
Maybe replace Region by a MultiIndex template (defaulted to Region) instead? That would also avoid the usage of a nullable.

@kilianvolmer
Copy link
Contributor Author

Two comments on this:

Fixing the copyright notice is entirely out of scope of this PR. That would be fine, if the changes was limited to a file with other modifications, which is not the case. Please move the copyright changes to a new PR.

I already moved the changes to the docs branch, that will be merged first.

I guess gluing on the group indices works, but having to use std::apply whenever we need to index anything is frankly an awful user experience. Can you find a different solution? Maybe replace Region by a MultiIndex template (defaulted to Region) instead? That would also avoid the usage of a nullable.

So far, this index is used exactly twice and I can't think of a reason to use it in the "frontend". Thus I would argue that the overhead of using this is manageable, especially as it doesn't require any other changes to existing code.

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.

SMM model with more indices
2 participants