Implement improvements to the Manager class

[nickbianco]

Fixes issue #4089

Summary

This PR represents a major overhaul of the Manager class. These changes are intended largely preserve the existing basic behavior while adding several useful new features and make usage much clearer to users. The documentation has been mostly rewritten to better explain proper usage of Manager. Some features are deprecated or removed in the current draft, but these changes are still up for discussion (see #4089 for more details).

Merging should wait until the following Simbody PR is merged: simbody/simbody#825.

List of changes

• Support for user-specified time step sequences (e.g., setUseSpecifiedDT, useConstantDT, etc.) have been removed. (See discussion below about possibly retaining these features).
• Support for halting an integration (Manager::halt()) has been removed. (See discussion below about possibly retaining this feature).
• Support for a user-provided Storage object has been removed.
• The Model can now only be set via the constructors and cannot be changed later (i.e., Manager::setModel()).
• The implementation of Manager::integrate() has been revamped and largely simplified. Changes to the internal SimTK::Integrator no longer occur when calling Manager::integrate()
• Manager can now be re-initialized, meaning that, after making a changes to the State in a simulation loop, users can now just call Manager::initialize() rather than constructing a new Manager entirely. This matches the behavior of SimTK::TimeStepper.
• The SimTK::TimeStepper is now initialized from the Manager constructors and can only changed when the integrator is changed via Manager::setIntegratorMethod().
• SimTK::CPodesIntegrator is now supported. It can be specified using the Manager::IntegratorMethod::CPodes enum.
• Added an option to record a full StatesTrajectory (via Manager::setRecordStatesTrajectory). This differs from the behavior of StatesTrajectoryReporter in that all internal time steps are recorded, rather than states at a user-provided time interval.
• Convenience methods for a few advanced SimTK::Integrator settings have been added (e.g., Manager::setIntegratorFinalTime).
• Manager::record() is now invoked when the status SimTK::Integrator::ReachedReportTime is returned. (I'm not sure we actually want to do this, see notes below).
• ForwardTool::run() has been updated to maintain support for the setUseSpecifiedDt setting.

Testing I've completed

The test suite in testManager.cpp has been expanded and upgraded to the Catch2 framework. testForward.cpp has also be converted to Catch2, and a new subtest has been added to test that setUseSpecifiedDt works as expected.

Looking for feedback on...

To create this draft, it was easiest to remove the user-provided time step sequence methods (e.g., setUseSpecifiedDT, useConstantDT, etc.), as these were the main reason that the implementation of Manager::integrate was muddled. However, support for these methods does not necessarily need to be removed. It would be ideal to keep the simplified time-stepping logic intact, but this could be moved to a helper function (e.g., Manager::initializeInternal) which could be called multiple times internally given the user-specified sequence of time steps.

Manager::halt could easily be reimplemented, but I'm not sure why it exists in the first place. It's not used anywhere in OpenSim and is not tested. It may exist for use in real-time applications or the GUI, but I'm not sure.

Previously, if you added a StatesTrajectoryReporter to the model passed to Manager, it would record states at the interval specified in the reporter, but these states would not necessarily be recorded in the internal Storage. This seemed odd to me, so I added a change to invoke Manager::record() each time the status SimTK::Integrator::ReachedReportTime is returned. Is it worth it to add the extra Manager::record() calls to for the sake of consistency, or should we restore the original behavior?

CHANGELOG.md (choose one)

• no need to update because...
• updated.

---

This change is 

[nickbianco]

@carmichaelong I've created a new PR on a separate branch to clean up the git commit history. Below is your previous comment from the other PR.

---

Overall, the changes to streamline the Manager class (e.g., interface in the header file) look great. Some specific responses to discussion points from above:

User provided time stepping sequences
I think the overwhelming majority of reasonable use cases are either fixed-step or error-controlled. Given this, it could make sense to remove user-facing functions for providing specific time-stepping sequences (i.e., from both Manager and ForwardTool). If we want to keep it, it could be added back to Manager, but a new interface would be helpful.

Manager::halt()
Agreed, I could not find any use cases. It would be good to double check if the GUI needs halt().

Recording states at reporter's intervals
It could be helpful to figure out the exact goals of different classes. For instance:

• Manager should focus on integrating quickly and efficiently
• Outputs can be used to see into the current simulation, such as to feed back states to an optimization problem
• Reporters can be attached to get simulation details out at specified times. This could be interpolated after the simulation, if needed.
• Many users may not need to know any of these details, and often the behavior of a Reporter is used the most. Since details of a Reporter may not be important, a Reporter could use other helpful classes (e.g., Output) to do its job without a user being aware of it, but maintaining speed.

[nickbianco]

Thanks for the comments @carmichaelong. Seems like we're on the same page; I'll start incorporating a few changes including deprecating the option to provide a time step sequence in ForwardTool.

@aymanhab, do you know if Manager::halt() has any use in the GUI?

[aymanhab]

@nickbianco GUI does not use halt, it uses a callback/interrupt to stop the tool. It doesn't use the manager directly in any capacity.

[nickbianco]

@tkuchida I tagged you for a secondary review here since you took a look over the previous draft. Let me know if you don't have time for a full review, I can find an alternate.

[aymanhab]

Reviewed 2 of 9 files at r1, all commit messages.
Reviewable status: 2 of 9 files reviewed, 6 unresolved discussions (waiting on @tkuchida)

---

OpenSim/Simulation/Manager/Manager.h line 86 at r1 (raw file):

 *
 * Manager::initialize() must be called before calling Manager::integrate(). A
 * Manager may be reinitialized by calling initialize() with a different

Do we enforce this "must" behavior by throwing otherwise?

---

OpenSim/Simulation/Manager/Manager.h line 273 at r1 (raw file):

     */
    void setIntegratorMinimumStepSize(double hmin);

I had the impression we want to free the hand of the integrator to take arbitrary step sizes, so setting min and max runs counter the idea but there maybe practical reasons for it?

---

OpenSim/Simulation/Manager/Manager.h line 308 at r1 (raw file):

     */
    void setIntegratorFixedStepSize(double stepSize);

Again I understand fixed reporting but fixed stepping is a bit murky if it conflicts with error control

---

OpenSim/Simulation/Manager/Manager.h line 459 at r1 (raw file):

     */
    Storage getStateStorage() const;

Generally we were moving away from Storage and into TimeSeriesTable, is that still here because TimeSeriesTable can't handle mixed types? Should that be maintained in the new interface?

---

OpenSim/Simulation/Manager/Manager.h line 495 at r1 (raw file):

     */
    void setModel(Model& model);

Why keep these deprecated baggage methods when we're revamping and potentially breaking API?

---

OpenSim/Simulation/Manager/Manager.h line 692 at r1 (raw file):

    std::unique_ptr<Storage> _stateStore;

    // HELPER METHOD

old class maintained ControllerSet, can you comment on why was it there and was removed here? Are there implications outside the manager class, or use cases you're aware of?

[aymanhab]

@nickbianco This looks much cleaner already, I just had a few questions to clarify after reviewing Manager.h, the rest will follow after I get your feedback. If already explained elsewhere I'd appreciate a pointer. Thank you 👍

Reviewable status: 2 of 9 files reviewed, 6 unresolved discussions (waiting on @nickbianco and @tkuchida)

[nickbianco]

Thanks @aymanhab, I've addressed all comments and pushed a few changes.

Reviewable status: 2 of 9 files reviewed, 6 unresolved discussions (waiting on @adamkewley, @aymanhab, and @tkuchida)

---

OpenSim/Simulation/Manager/Manager.h line 86 at r1 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Do we enforce this "must" behavior by throwing otherwise?

Yes, but I've updated the documentation and added an explicit test to be sure.

---

OpenSim/Simulation/Manager/Manager.h line 273 at r1 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

I had the impression we want to free the hand of the integrator to take arbitrary step sizes, so setting min and max runs counter the idea but there maybe practical reasons for it?

In most cases yes, and that should be the default behavior. But putting an upper or lower bound on the step size that the integrator can take is also valid behavior. The main thing we want to avoid encouraging users to use a prescribed sequence of time steps.

I added a note here and above to make this clearer to users.

---

OpenSim/Simulation/Manager/Manager.h line 308 at r1 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Again I understand fixed reporting but fixed stepping is a bit murky if it conflicts with error control

It does conflict with error control, but users may still want this option. The note in the documentation is intended to make clear how enabling this setting could affect results.

---

OpenSim/Simulation/Manager/Manager.h line 459 at r1 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Generally we were moving away from Storage and into TimeSeriesTable, is that still here because TimeSeriesTable can't handle mixed types? Should that be maintained in the new interface?

I wasn't sure about removing the support for Storage or not. One reason for keeping the Storage option is that it is faster than recording a StatesTrajectory (and likely also a TimeSeriesTable). I think the main reason is that Storage preallocates memory.

If we wanted to eventually move away from Storage we could try storing a minimal trajectory of states using a custom lightweight container internally, and then convert to whatever output we want.

---

OpenSim/Simulation/Manager/Manager.h line 495 at r1 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Why keep these deprecated baggage methods when we're revamping and potentially breaking API?

Good question. I think the idea is to indicate the removal of these features while not breaking builds. To be honest, I was following @adamkewley's lead based on some recent changes to Array.

I could also see arguments for just removing them.

---

OpenSim/Simulation/Manager/Manager.h line 692 at r1 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

old class maintained ControllerSet, can you comment on why was it there and was removed here? Are there implications outside the manager class, or use cases you're aware of?

I removed it because it didn't seem necessary: we can always access the ControllerSet directly from the reference to the Model.