Store control-system measurement times in the DataBox

[wthrowe]

Knowing the times will be necessary for implementing time-step adjustment to avoid deadlocking.

Proposed changes

Upgrade instructions

Code review checklist

• The code is documented and the documentation renders correctly. Run
  make doc to generate the documentation locally into BUILD_DIR/docs/html.
  Then open index.html.
• The code follows the stylistic and code quality guidelines listed in the
  code review guide.
• The PR lists upgrade instructions and is labeled bugfix or
  new feature if appropriate.

Further comments

[knelli2]

Can you expand on this a bit? What could this potentially be used for (what is it intended to be used for)? Maybe say that this is also aware of updates as well as measurements?

[wthrowe]

I've expanded a little, but I don't really have much to say. I generally prefer to avoid listing what uses a class in its documentation unless things are really tightly coupled, because that's not a property of the class and can end up out-of-date very easily.

Do you have a suggested wording for a statement about updates? The "time of an update" seems to be ambiguous, since the SpEC update got split into two things happening at different times and I haven't observed a consensus about which that phrase would refer to.

[knelli2]

I think your fixup was a good \brief for a class, but it basically just repeated what the namespace and the class name already said: control_system::FutureMeasurements = class that knows about future control system measurements. In my opinion that's not super helpful documentation.

I view the docs as me (or an anthropomorphized version of the class) giving a newer dev of the code a (brief) explanation of how this class ties in to the surrounding infrastructure. You don't have to give specific uses of this class like "this class will be used in the control_system::Trigger class for ...". I agree that's hard to maintain. But you can give a broader idea of why this class is useful.

Here's how I would have documented the class. If you decide to use it, feel free to change up some wording. Hopefully the last paragraph removes the ambiguity of what a "time of an update" means.

Class for computing the upcoming measurement times for a control
system measurement.

The measurement timescales are stored in the GlobalCache. At a given simulation time,
they can be used to get the current measurement timescale. However, it is
sometimes advantageous to know when future measurements will occur in
case preemptive decisions have to be made. It is difficult to know how to
calculate these future measurements without additional information and
knowledge about the structure of the measurement timescales. This class
has this additional knowledge and takes care of that calculation. See the
`control_system::Tags::MeasurementTimescales` tag for this additional
information.

In addition to knowing about the simulation time of future measurements,
this class also knows the simulation time of control system/FunctionOfTime
updates as well.

[wthrowe]

I think your fixup was a good \brief for a class, but it basically just repeated what the namespace and the class name already said: control_system::FutureMeasurements = class that knows about future control system measurements. In my opinion that's not super helpful documentation.

I added that it computes the values, which is an important point that was missing.

The measurement timescales are stored in the GlobalCache.

I don't want to talk about the GlobalCache here. Avoiding use of the GlobalCache in this class was an intentional decision I made.

At a given simulation time,
they can be used to get the current measurement timescale. However, it is
sometimes advantageous to know when future measurements will occur in
case preemptive decisions have to be made.

These two sentences are good. I'll use something like them.

It is difficult to know how to
calculate these future measurements without additional information and
knowledge about the structure of the measurement timescales. This class
has this additional knowledge and takes care of that calculation. See the
`control_system::Tags::MeasurementTimescales` tag for this additional
information.

It's not factored out because it's difficult or confusing. It's just t + timescale(t). It's factored out because it seems like a bad idea to calculate and store all the values in two places.

Although, now that I poke around, I don't actually see t + timescale(t) documented anywhere, and this does seem like a pretty good place for it.

Linking to the tag is a good idea, too.

In addition to knowing about the simulation time of future measurements,
this class also knows the simulation time of control system/FunctionOfTime
updates as well.

That's actually the "time of update" that it doesn't know about. It knows about the measurement half.

[knelli2]

You can squash the docs changes directly if you decide to make them.

[knelli2]

I think your fixup was a good \brief for a class, but it basically just repeated what the namespace and the class name already said: control_system::FutureMeasurements = class that knows about future control system measurements. In my opinion that's not super helpful documentation.

I view the docs as me (or an anthropomorphized version of the class) giving a newer dev of the code a (brief) explanation of how this class ties in to the surrounding infrastructure. You don't have to give specific uses of this class like "this class will be used in the control_system::Trigger class for ...". I agree that's hard to maintain. But you can give a broader idea of why this class is useful.

Here's how I would have documented the class. If you decide to use it, feel free to change up some wording. Hopefully the last paragraph removes the ambiguity of what a "time of an update" means.

Class for computing the upcoming measurement times for a control
system measurement.

The measurement timescales are stored in the GlobalCache. At a given simulation time,
they can be used to get the current measurement timescale. However, it is
sometimes advantageous to know when future measurements will occur in
case preemptive decisions have to be made. It is difficult to know how to
calculate these future measurements without additional information and
knowledge about the structure of the measurement timescales. This class
has this additional knowledge and takes care of that calculation. See the
`control_system::Tags::MeasurementTimescales` tag for this additional
information.

In addition to knowing about the simulation time of future measurements,
this class also knows the simulation time of control system/FunctionOfTime
updates as well.

[wthrowe]

Pushed fixups. Changes:

• Documentation update discussed above. I decided to put the update formula on the update method.
• Changed inactive control systems to report one measurement per update. I found this more convenient for code in a later PR.
• Changed the assertion about the list growing long to a non-error limit on the list size. The class never uses more than the first cycle, but storing a bit more means no thought has to go into the exact number of entries required. I also got worried that it might try to store every measurement time during a wave-zone step in LTS.

[knelli2]

Looks good. Squash

[knelli2]

Not sure what some of the test failures are because I can't see the output. But they failed building the executables

[wthrowe]

For gcc-9 Release, "View raw logs" shows "g++-9: fatal error: Killed signal terminated program cc1plus", so presumably OOM. Doesn't work for gcc-10 Release, though.

[knelli2]

Ok sounds good. Nothing to block this PR