-
Notifications
You must be signed in to change notification settings - Fork 19
977 remove t0 from simulation class and allow flexible start times in model #979
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.
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
977 remove t0 from simulation class and allow flexible start times in model #979
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #979 +/- ##
=======================================
Coverage 96.34% 96.34%
=======================================
Files 129 129
Lines 10056 10071 +15
=======================================
+ Hits 9688 9703 +15
Misses 368 368 ☔ View full report in Codecov by Sentry. |
lenaploetzke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work, I liked the test too!
At the moment the m_transitions TimeSeries is public, so we can change the TimeSeries without confirming that the last value here matches the last value in m_populations. Can you maybe write a setter+getter and make m_transitions private?
|
I see the problem that m_transitions can be changed without confirming that the last value there matches the last value in m_populations. I can implement a setter where we would reset m_populations accordingly. Then m_populations should be set private, too, which would making setting S and R for the initialization of the solver a bit more tedious. I'm not sure if this is worth it or if it would be enough to add a constraint to the check_constraints function. |
We discussed this and agreed to stay with the public members and implement an additional check in the check_constraints() function. |
Co-authored-by: lenaploetzke <70579874+lenaploetzke@users.noreply.github.com>
lenaploetzke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good!
Changes and Information
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
Checks by code reviewer(s)
Closes #977