Class StatesDocument

[fcanderson]

Brief summary of changes

Class StatesDocument is an addition to OpenSim that adds the ability to serialize and deserialize a complete time history of model states to and from a single .ostates file. This class fulfills a roadmap articulated about 10 years ago. See the bottom of StatesTrajectory.h.

Prior to class StateDocument, only the continuous states (e.g., joint coordinates, joint speeds, etc.) were serialized in a systematic manner, leaving discrete states and modeling options unserialized. One of the challenges in serializing discrete states is that their type can vary (e.g., bool, int, double, Vec2, Vec3, etc.) and existing approaches for serializing states (e.g., storage files and time series tables) do not easily support data of mixed types.

Class StatesDocument uses XML (see class SimTK::Xml) as the framework for serializing all state categories and all data types. It also contains serialization date stamps accounting for time zone, contains an annotation note, and supports serialization with a range of output precisions ( 1 to 20 digits), allowing the user to reduce the .ostates file size or achieve lossless de/serialization, .

Minor additions were made to class StatesTrajectory and StatesTrajectoryReporter to support the StatesDocument class. Note, however, that class StatesDocument is build on low-level SimTK classes and the recently enhanced OpenSim::Component class, and does not have any knowledge of the StatesTrajectory or StatesTrajectoryReporter classes.

For details concerning class StatesDocument, consult the Doxygen compliant comments in StatesDocument.h.

Testing I've completed

Class StatesDocument is accompanied by a unit test. See testStatesDocument.cpp. This unit test exercises state serialization in all categories (continuous variables, discrete variables, and modeling options) and in all supported data types (bool, int, double, Vec2, Vec3, Vec4, Vec5, Vec6). All OpenSim unit tests are passing on Windows 10 for a RelWithDebInfo build.

Looking for feedback on...

• Are modifications to StatesTrajectory and StatesTrajectoryReporter acceptable?
• Should the unit test include more comprehensive checks of exception handling and a broader range of models?

CHANGELOG.md (choose one)

CHANGELOG.md has been updated.

---

This change is 

[aymanhab]

Thanks @fcanderson Can we fix build failures to start. Thank you

[fcanderson]

Hi @aymanhab. Thanks for the reply. Sorry for the long radio silence. I got really busy with the class I'm teaching. I believe the errors occur only during the wrapping (Java, Python, ...). I'm guessing I need to insert some compiler directives that prevent a wrapping attempt. I may have time to look into that this morning, as long as I don't need to put out any fires.

[aymanhab]

@fcanderson No problem, it's just an issue of missing default constructor I think, which I can explicitly communicate to SWIG but I wasn't sure if that's the intended behavior. Let me know if you get busy and I can investigate further. Thank you

[fcanderson]

I'll create a default constructor. It won't be useable for anything meaningful, but it might make the errors go away.

[fcanderson]

@aymanhab I added a default constructor. Continuous Integration now performs the wrapping successfully. At least the build/testing is getting farther.

The two tests that fail are: 132 - python_tests (Failed) 133 - python_examples (Failed)

I haven't been doing the python wrapping on my desktop. I will look into doing that. Do you have time to take a look to see if there is a quick fix? If not, I'll do some digging.

[aymanhab]

No worries @fcanderson I'll take a look and let you know if I have questions. Thank you 👍

[fcanderson]

Ok! Thanks @aymanhab.

[aymanhab]

Failure details:

ERROR: test_index_and_iterator (test_states_trajectory.TestStatesTrajectory)

Traceback (most recent call last): File "D:\dev\src\opensim-core\Bindings\Python\tests\test_states_trajectory.py", line 45, in test_index_and_iterator model.calcMassCenterPosition(state) File "D:\dev\build\opensim-core\Bindings\Python\Release\opensim\simulation.py", line 25970, in calcMassCenterPosition return _simulation.Model_calcMassCenterPosition(self, s) ValueError: invalid null reference in method 'Model_calcMassCenterPosition', argument 2 of type 'SimTK::State const &'

[aymanhab]

@fcanderson The error happens while iterating through the stateTrajectory due to the change from std::vector into SimTK::Array in stateTrajectory.h is there a reason for the change? std::vector iterators are likely better supported by swig out of the box than our home grown SimTK::Array, but if the change is necessary I'll dig deeper into the iterator across bindings. Thank you

[aymanhab]

swig/python detected a memory leak of type 'SimTK::Array_< SimTK::State,unsigned int >::constiterator *', no destructor found. swig/python detected a memory leak of type 'SimTK::Array< SimTK::State,unsigned int >::const_iterator *', no destructor found.

[fcanderson]

@aymanhab Thanks for tracking the origin of this error down.

The reason for the transition is that Simbody uses SimTK::Array for its underlying std::vector-like functionality. The original rationale (I assume coming from Sherm) was for some speed and for binary compatibility. The OpenSim::StatesDocument class uses SimTK::Array to be compatible with Simbody, as do a number of newly available trajectory methods in OpenSim::Component.

I get why swig would be more likely to handle std::vector properly than SimTK::Array. One option might be to hide the underlying implementation (vector vs. Array) so that python / java / etc. don't see it. For example, by just using an index instead of an iterator -- a little slower sometimes, but I don't think noticeably.

I will look into this issue and see if I can come a good/easy solution.

[aymanhab]

Thanks @fcanderson My first inclination would be to restore the stateTrajectory class to use std::vector for backward compatibility of client code, so that iterators (which are used by the stateTrajectory class clients) are maintained. The speed up in SimTK::Array_ may be important for small arrays internal to simbody (where space efficiency matters) but std::vector is used extensively elsewhere and the State object is massive enough that overhead would be negligible I'd think. I can try that myself if you agree so please let me know..

[fcanderson]

Agreed. I will revert to using std::vector. It will have ripple effects in the StatesDocument class and maybe in the Component trajectory related classes, but the number of changes are finite.

[fcanderson]

@aymanhab I'm following up after looking into the issue of std::vector<> vs. SimTK::Array_<> a bit more.

There are some compelling performance differences between the two, with SimTK::Array_<> being about 2 times faster. There are also some undesirable characteristics of std::vector<> having to do with memory and binary compatibility, particularly for Microsoft implementations.

The Simbody documentation goes into the details here: SimTK::Array_<> Details. It looks like Sherm et al. invested significant effort into the SimTK::Array_<> class.

The differences are not just a matter of overhead. For example, simple operations like indexing through an array are 2 times slower for std::vector. The larger the array, the more the performance difference will be noticeable.

Since state trajectories may be fairly large at times, my leaning is to keep the innards of OpenSim::StatesDocument and the associated trajectory methods in OpenSim::Component based on SimTK::Array_<>.

To interface with the outside world, I propose simply adding a thin translation layer to OpenSim::StatesDocument that will translate a std::vector<SimTK::State> object into a SimTK::Array_<SimTK::State> object.

There are only two methods in OpenSim::StatesDocument where the translation would need to occur:

StatesDocument::StatesDocument(const Model& model, const Array_<State>& trajectory, const String& note, int p)
StatesDocument::deserialize(const Model& model, Array_<State>& traj)

I propose that we just add two additional methods so that OpenSim::StatesDocument can communicate with classes like OpenSim::StateTrajectory that use std::vector<SimTK::Array_>.

So the API would be:

StatesDocument::StatesDocument(const Model& model, const vector<State>& trajectory, const String& note, int p)
StatesDocument::StatesDocument(const Model& model, const Array_<State>& trajectory, const String& note, int p)

StatesDocument::deserialize(const Model& model, vector<State>& traj)
StatesDocument::deserialize(const Model& model, Array_<State>& traj)

The vector-based methods would need to perform a copy from vector<State> to Array_<State> (or vice versa), but that would only be done once and the cost would be small (I think) compared to the cost of iterating through all of the individual continuous, discrete, and modeling variables that make up the State using vector instead of array.

[fcanderson]

@aymanhab I will add that it looks like I can use the following constructor:

SimTK::Array_<T>::Array_ (std::vector<T> &v, const DontCopy)

which generates an Array_<T> that directly references (does not copy) the data of the vector argument. This constructor is dirt cheap and I believe a safe thing to use in the circumstance of the StatesDocument class.

[aymanhab]

Great investigation as always @fcanderson 👍 I guess clients of the class most always do iteration rather than indexing so this didn't factor in the design of stateTrajectory. I'm totally on board with making the interface be std::vector and the internals using SimTK::Array, we get the best of both worlds.

[fcanderson]

@aymanhab Ok! I reverted back to std::vector<State>, as opposed to SimTK::Array_<>, in OpenSim::StateTrajectory and in OpenSim::StatesTrajectoryReporter, and the tests are passing. Thanks for identifying the issue!

The modifications to allow OpenSim::StatesDocument to accept std::vector<State>, in addition to SimTK::Array_<State>, were not bad. The shallow constructor SimTK::Array_<std::vector<State>, SimTK::DontCopy()) worked like a charm. Internally, OpenSim::StatesDocument continues to use SimTK::Array_<State>, which will be computationally faster and work more seamlessly with Simbody.

[fcanderson]

OpenSim/Simulation/StatesDocument.h line 221 at r6 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Amazingly clear description @fcanderson Thank you :1st_place_medal:

Thank you. I worked hard on the documentation. Understanding how states work in Simbody and in OpenSim is not generally well understood, even in the developer community. It was an opportunity to get a clear description out there. In addition, it was important to lay out the foundational need for a class like OpenSim::StatesDocument.

[fcanderson]

OpenSim/Simulation/StatesDocument.cpp line 450 at r6 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

This maybe overstated on my side but we never gave the model name much significance, users easily modify model names in GUI or XML without thinking about downstream effects. Considering that the meat of consistency test is done elsewhere I wonder if you think this check is relevant here.

You are in a better position to make this call.

In an effort to avoid a situation where a user would perform a detailed analysis with the wrong set of states, I opted to make the bar for compatibility high. If a user really would like to conduct an analysis with a particular .ostates file that has a model name that doesn't match, I thought opening the file and manually changing the file name, though tedious, was not too much to ask.

You are familiar with use cases in the GUI, Matlab, etc. What do you suggest?

[fcanderson]

OpenSim/Simulation/StatesDocument.cpp line 673 at r6 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

These if/else(s) typically worry me because I don't know if we covered all cases. but with templates I guess that's the price to pay. Is there a master list of these types that we can refer to in case e.g. Arrays of Vectors, Matrices or SpatialVectors beome valid types?

I thought about ways of avoiding the if/else(s), but did not come up with something that wasn't burdensome. OpenSim has been used for YEARS and no one until I came along with my contact class has complained about other types besides double being unavailable.

I thought when another such case comes along, we could quickly add another if/else for the type in demand. It really only means the addition of several lines of code.

To clarify, in Simbody, any numerical type that can be represented by a SimTK::AbstractValue() is fair game for a SimTK::DiscreteVariable.

In OpenSim::StatesDocument, the supported types are listed in the documentation: https://github.com/fcanderson/opensim-core/blob/ostates_2/OpenSim/Simulation/StatesDocument.h#L278

[fcanderson]

The following answer may not address your question adequately. I find it challenging to answer. If I come up short, a Zoom call in which you and I can go back and forth about concepts might be the easiest way to proceed.

API users should always assume that every instance of a SimTK::State that they encounter is comprised of continuous variables, discrete variables, and modeling options. For example, a joint clamp is implemented in OpenSim and Simbody using a modeling option.

For the purpose of state de/serialization,OpenSim::StatesDocument is the preferable path to take. Other approaches for de/serialization will leave any discrete states and modeling options uninitialized, unless additional measures are taken.

Historically, API programmers have taken those additional measures. When states other than the continuous states have been needed, those states have been saved in some ad hoc way. For example, as a Property in the OpenSim model file or as separate input files (e.g., the time history of muscle excitations). Users just haven't been aware that the need to do this, in some cases, stems from an inability to save the full state to file. API users have always been aware of their particular needs and so have programmed approaches that stepped around the fact that OpenSim was only able to save the continuous states to file.

The situation with my contact model targeted the Achilles' Heel of OpenSim. My contact model not only has input discrete states of various types (int, Vec3, etc) , it also has OUTPUT discrete states of different types! There just wasn't a viable way to handle this in OpenSim.

This whole area has been hard to communicate about. Nick has an appreciation of the details; he performed a detailed review of the enhacements to OpenSim::Component. In the Community Dev meetings, there's really only time for bullet points, not time to take a deep dive into the sometimes difficult finer points. And I'm not always the best communicator.

I see the OpenSim workflow as migrating to something like:

1. Open a model or create a model.
2. Perform a simulation (or some other kind of model-based computation).
3. Obtain the full state trajectory.
4. For future analysis (if desired), serialize the full trajectory of states using OpenSim::StatesDocument.
5. For post analysis, deserialize the full trajectory of states using OpenSim::StatesDocument.
6. Perform analyses, visualize movement, visualize results, perform additional simulations, etc.
7. Write desired results to time series tables for plotting and/or other analyses that don't require the model.

I haven't seen the .ostates XML file as the end product from the user's point of view. I've seen it as a systematic and comprehensive means of getting at the information they desire. Because .ostates will always contain the full state, any piece of data concerning a model can be (re)generated.

[fcanderson]

OpenSim/Simulation/StatesDocument.cpp line 450 at r6 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

I'll leave it as is and see if it becomes an issue in actual use cases, at this level of API use, more stringent is better.

agreed

[fcanderson]

OpenSim/Simulation/StatesDocument.cpp line 673 at r6 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Thanks @fcanderson I'd add comment to the block in the cpp block so that future maintainers understand where this list came from and why.

Working on it.

[fcanderson]

OpenSim/Simulation/StatesTrajectoryReporter.cpp line 39 at r5 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Agreed

Done.

[fcanderson]

OpenSim/Simulation/StatesDocument.cpp line 435 at r6 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

parsed?

Done.

[fcanderson]

@aymanhab I just updated CHANGELOG.md. Thank you very much, Ayman, for your review. I am glad to have this chunk of functionality in OpenSim. Now that auto-update discrete states of different variable types are supported in OpenSim, the next stop is getting the ExponentialContact class merged and seeing how it performs for real models not just simple test models.