Closed reneSchm closed 4 months ago
I fixed the issue using another version of the try_step method. However, this breaks the runge_kutta_dopri5 stepper due to a bug in boost. Since we currently only use it in testing and benchmarks, I decided to prevent it (and other FSAL steppers) to be used in the wrapper.
@mknaranja Do you think its okay to disable FSAL steppers for now? Otherwise, the ControlledStepperWrapper would have to make a manual integration step whenever the normal try_step loop fails. This is not a terrible solution, but we potentially make a repeated integration step whenever dt==dt_min.
The test TestOdeSecir.compareAgeResWithPreviousRunCashKarp currently fails since it uses an integration result with a time step < dt_min as reference. @MaxBetzDLR is it safe to just update the csv with a new integration result?
Currently, both ControlledStepperWrapper and RKIntegratorCore are allowed to make steps of size > dt_max, only if the function argument dt > dt_max. We could either keep this behaviour, I don't think it will matter much or have a negative impact. But it may be unexpected, so we could warn/exit on a bad input; or allow any input value, but restrict it to [dt_min, dt_max] internally. I would prefer restricting dt internally, as we already enforce the lower bound of dt_min.
Attention: Patch coverage is 97.67442%
with 1 lines
in your changes are missing coverage. Please review.
Project coverage is 96.23%. Comparing base (
058b3cc
) to head (31f4ae7
). Report is 3 commits behind head on main.
Files | Patch % | Lines |
---|---|---|
cpp/memilio/math/integrator.cpp | 0.00% | 1 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Currently, both ControlledStepperWrapper and RKIntegratorCore are allowed to make steps of size > dt_max, only if the function argument dt > dt_max. We could either keep this behaviour, I don't think it will matter much or have a negative impact. But it may be unexpected, so we could warn/exit on a bad input; or allow any input value, but restrict it to [dt_min, dt_max] internally. I would prefer restricting dt internally, as we already enforce the lower bound of dt_min.
@reneSchm This is not good design we did. We should not allow that, adapt it and inform/warn on it.
One comment. Should we also adjust the default values for the minimum step sizes? (also in stepper_wrapper.h)
I don't think we should set a different dt_min, which scale would we even choose?
Maybe we could set a dt_min in the simulation, but I would prefer to keep it model/executable specific.
So I added a small warning when the given dt was out of bounds, and strangely enough it might have improved performance for some steppers?
RKIntegratorCore
aka adapt_rk and ControlledStepperWrapper<runge_kutta_fehlberg78>
aka rkf78 are consistently about 50ns faster. Which is nice, but confusing. Apart from the couple lines for the logging, the assembly looked like only some addresses got reordered, at least I didn't see any significant changes. My best guess is that I got lucky and some reordering caused better cache hits, but I don't know.ControlledStepperWrapper<runge_kutta_cash_carp54>
aka rk_ck54 seems to be roughly 10ns slower, but that is within the margin of error for these measurements. This is as expected, and I think well acceptable.Before logging (dae153c):
simulate SecirModel adapt_rk 1008 ns 1007 ns 696182
simulate SecirModel boost rk_ck54 746 ns 746 ns 938309
simulate SecirModel boost rkf78 2196 ns 2195 ns 317635
With logging (b93c0e6):
simulate SecirModel adapt_rk 935 ns 934 ns 749251
simulate SecirModel boost rk_ck54 759 ns 759 ns 921252
simulate SecirModel boost rkf78 2141 ns 2141 ns 323717
now the integrator makes a time step of at least dt_min, instead of potentially doing nothing except decreasing dt without limit. this breaks fsal steppers (in part. rk-dopri5, only used in tests and benchmarks), so they were disabled for now.
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 #949