SciCompMod / memilio

Modular spatio-temporal models for epidemic and pandemic simulations
https://scicompmod.github.io/memilio/
Apache License 2.0
51 stars 15 forks source link

977 remove t0 from simulation class and allow flexible start times in model #979

Closed annawendler closed 2 months ago

annawendler commented 3 months ago

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

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 96.34%. Comparing base (a5cbb91) to head (59ffa22). Report is 8 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #979 +/- ## ======================================= Coverage 96.34% 96.34% ======================================= Files 129 129 Lines 10056 10071 +15 ======================================= + Hits 9688 9703 +15 Misses 368 368 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

annawendler commented 2 months ago

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. Another possibility would be to set m_transitions private anyway without implementing getter and setter. Then we could define a friend class to set the initial flows based on real data or need to find another solution.

lenaploetzke commented 2 months ago

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. Another possibility would be to set m_transitions private anyway without implementing getter and setter. Then we could define a friend class to set the initial flows based on real data or need to find another solution.

We discussed this and agreed to stay with the public members and implement an additional check in the check_constraints() function.