Allow specifying 'None' for control sys in input file #6482
Conversation
src/Parallel/CreateFromOptions.hpp
Outdated
| [](const auto&... option) { | ||
| return Tag::template create_from_options<Metavariables>(option...); | ||
| return Tag::template create_from_options<Metavariables>( | ||
| get_option(option)...); |
There was a problem hiding this comment.
I don't like special-casing Auto here. It isn't explicitly converted in the more common option parsing locations, and this should be kept consistent with those.
It is unfortunate that C++ can't figure out to do the implicit conversion in some cases, but I think it is the job of the class requesting an Auto to be able to handle receiving one.
There was a problem hiding this comment.
Yeah this was very unfortunate that it couldn't figure it out. I'll remove this commit and try some other magic in the tags themselves.
src/ControlSystem/TimescaleTuner.hpp
Outdated
| DataVector timescale_; | ||
| bool timescales_have_been_set_{false}; | ||
| double initial_timescale_{std::numeric_limits<double>::signaling_NaN()}; | ||
| double initial_timescale_; |
There was a problem hiding this comment.
I don't understand what this commit is doing.
There was a problem hiding this comment.
If you default constructed a TimescaleTuner and then compared it with ==, you'd get an FPE because of the signaling NaN.
There was a problem hiding this comment.
So now you're comparing uninintialized memory instead?
59c6ec0 to
51ad0b6
Compare
| const OptionHolders&... option_holders) { | ||
| return create_from_options<Metavariables>( | ||
| domain_creator, measurements_per_update, initial_time, | ||
| detail::get_option_holder(option_holders)...); |
There was a problem hiding this comment.
Are these ever going to be anything except Autos? If not, just cast them to optionals, or add a method to Auto to do the cast so you don't have to deal with the template argument.
|
Looks good. Squash. |
This simplifies the input files that don't use control systems a lot
5c92b5f to
35f13a8
Compare
Proposed changes
If a control system is inactive, it doesn't make a ton of sense to have to specify parameters that will never be used. This implements a similar system to the time dependent options for the BCO and Sphere where you can specify
Nonefor a control system to disable it. This also decreases the length in input files like BNS which won't use most control systems.There was no functionality change in how the control system works. This was purely cosmetic.
Upgrade instructions
To turn a control system off in the input file, use
When keeping a control system active, keep all the same previous options, except remove the
IsActive:option. I.e.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