Allow timescale tuner to not decrease timescale #5583
Conversation
|
|
||
| template <typename T> | ||
| struct has_signature_1< | ||
| T, std::void_t<decltype( |
There was a problem hiding this comment.
[optional] This looks very similar to std::is_invocable_r. Might be able to simplify.
There was a problem hiding this comment.
That made things a lot cleaner, thanks!
| @@ -24,6 +25,11 @@ namespace control_system { | |||
| /// their public member names and assigned to their corresponding DataBox tags. | |||
| template <typename ControlSystem> | |||
| struct OptionHolder { | |||
| private: | |||
| static constexpr bool is_size = | |||
| ::control_system::size::is_size_v<ControlSystem>; | |||
There was a problem hiding this comment.
[future work] It would be nice to not have everything in the control system infrastructure special-cased on size control. At some point maybe we should have control systems report what features they want with something like ControlSystem::allow_timescale_decrease.
There was a problem hiding this comment.
I agree. I'll hopefully put this into the control system protocol eventually, but was a bit too much work at the moment.
| const double increase_timescale_threshold, const double increase_factor, | ||
| const double decrease_factor) | ||
| const double decrease_timescale_threshold, const double decrease_factor) |
There was a problem hiding this comment.
Do some sort of check that these are reasonable in the no-decrease case. Check decrease_factor == 1.0, at least.
There was a problem hiding this comment.
Only did the decrease_factor == 1.0 check because I pass limits::max() for the decrease threshold and that's less intuitive of a default.
| test_equality_and_serialization(); | ||
| tmpl::for_each<tmpl::list<std::true_type, std::false_type>>( | ||
| [](const auto bool_v) { | ||
| constexpr bool AllowDecrease = tmpl::type_from<decltype(bool_v)>::value; |
knelli2
force-pushed
the
timescale_decrease
branch
from
f670c66
to
a454b0c
Compare
|
@wthrowe Squashed in the changes |
knelli2
force-pushed
the
timescale_decrease
branch
from
a454b0c
to
7f0a994
Compare
Proposed changes
For size control, the
DecreaseFactorof theTimescaleTunerhas to be 1.0 because other logic takes care of decreasing the timescale. If it isn't 1.0, the run will crash almost instantly. Therefore, it doesn't really make sense to allow this as an option for size control. This PR templatesTimescaleTuneron a bool toAllowDecrease. All other control systems have this bool astrue, but size control has it asfalse.Upgrade instructions
If your executable has a Size control system, remove the
DecreaseThreshold:andDecreaseFactor:options from the TimescaleTuner of the Size control systems. Note that you should keep these options for theSmootherTuneroption of Size control.Code review checklist
make docto generate the documentation locally intoBUILD_DIR/docs/html.Then open
index.html.code review guide.
bugfixornew featureif appropriate.Further comments