SciCompMod / memilio

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

1038 odeintegrator can run indefinitely #1046

Closed reneSchm closed 1 week ago

reneSchm commented 1 month ago

Changes and Information

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

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

Checks by code reviewer(s)

Closes #1038

codecov[bot] commented 1 month ago

Codecov Report

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

Project coverage is 96.24%. Comparing base (2994f76) to head (0716e48).

:exclamation: Current head 0716e48 differs from pull request most recent head 59f4f1a

Please upload reports for the commit 59f4f1a to get more accurate results.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1046 +/- ## ========================================== + Coverage 96.23% 96.24% +0.01% ========================================== Files 128 128 Lines 10852 10861 +9 ========================================== + Hits 10443 10453 +10 + Misses 409 408 -1 ```

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

lenaploetzke commented 3 weeks ago

With these changes, the application where the problem occurred works as expected. Will review the code soon.

reneSchm commented 3 weeks ago

Nevertheless, I still get the warning "Adaptive step sizing failed. Forced at least one integration step of size dt_min." if the last step is forced.

The message should only appear if a non-forced step failed to adapt, i.e. the stepper wanted to make a step smaller than dt_min. Are you sure this message is due to the last step?

reneSchm commented 3 weeks ago

@HenrZu @lenaploetzke I think I don't like the force_step_size approach anymore, it makes both the OdeIntegrator as well as the IntegratorCore weirdly complicated. I made another version in #1049, could you please check that one out instead?

reneSchm commented 1 week ago

Closed in favor of v2.