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 

[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.

[aymanhab]

Reviewed 2 of 8 files at r1, 1 of 2 files at r3, 1 of 1 files at r4, 2 of 6 files at r5, all commit messages.
Reviewable status: 6 of 10 files reviewed, 2 unresolved discussions (waiting on @fcanderson)

---

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

const std::vector<SimTK::State>&
StatesTrajectoryReporter::getStateArray() const {
    return m_states.getStateArray();

Wonder if the name of the method should change?

---

OpenSim/Simulation/Test/testStatesTrajectory.cpp line 580 at r5 (raw file):

    //    states[5];
    //#endif

Any reason to keep these comments?

[fcanderson]

Reviewable status: 5 of 10 files reviewed, 2 unresolved discussions (waiting on @aymanhab and @nickbianco)

---

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

Previously, aymanhab (Ayman Habib) wrote…

Wonder if the name of the method should change?

Ah, I think I see what you are suggesting. The return is now a vector<> instead of a SimTK::Array_<>.

I didn't want to use getStateTrajectory() since there is a StateTrajectory class.

The only reason I might not want to use getStateVector() is that name will likely conjure the notion of an object that contains the states at a single time stamp. A SimTK::State object itself is what some might think of as a state vector.

The name that I think is probably most accurate and least likely to be confused with some other entity in this arena is

getVectorOfStateObjects()

since an std::vector<> is returned.

If you agree, I'll go ahead and change the method name to getVectorOfStateObjects().

---

OpenSim/Simulation/Test/testStatesTrajectory.cpp line 580 at r5 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Any reason to keep these comments?

I do not know why I commented out these lines originally. So, I'm glad you spotted them. It likely had to do with the fact that testStatesTrajectory fails for a Debug build. I might have thought that commenting out those lines would allow the test to run successfully. Not sure, but if I did, that was wrong.

I just tested the code with those lines commented out and with those lines restored. Either way, the Debug build fails because of an array index out of bounds exception that is thrown while executing

SimTK_SUBTEST1(testFromStatesStorageInconsistentModel, pre40StoFname);  (testStatesTrajectory.cpp: line 799)

In a release build (either Release or RelWithDebInfo) the test passes because bounds checking is disabled.

Since I didn't write this test, the better thing to do restore the lines that I commented out. I'm not exactly sure what functionality they are testing.

As a general comment, there are a number of OpenSim tests that fail for a Debug build. I haven't reported them because I suspected that you all (the devs) were aware of the failures and had decided to pass on them as non-critical. After talking with @nickbianco , however, it seems that maybe running the tests for a Debug build isn't really part of the development cycle. So maybe you all are not aware of the Debug test failures?

I intended to follow up with @nickbianco on this, but I just haven't had the resources to do it.

[aymanhab]

A couple minor comments but otherwise looks great. My main question at the end of this was how would API users distinguish if they're using a Full state with discrete variable and modeling options vs. the traditional state that had only continuous variables. Maybe some documentation/disclaimers would do.

Reviewed 4 of 6 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @fcanderson)

---

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

significant figures would go unnoticed).

Amazingly clear description @fcanderson Thank you 🥇

---

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

    // Many other aspects are checked for consistency than just the model
    // name. Those are more easily checked as the doc is parced.

parsed?

---

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

        SimTK_ASSERT_ALWAYS(false, msg.c_str());
    }

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.

---

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

                model, path, traj);
        }
        else {

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?

---

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

Previously, fcanderson (Clay Anderson) wrote…

Ah, I think I see what you are suggesting. The return is now a vector<> instead of a SimTK::Array_<>.

I didn't want to use getStateTrajectory() since there is a StateTrajectory class.

The only reason I might not want to use getStateVector() is that name will likely conjure the notion of an object that contains the states at a single time stamp. A SimTK::State object itself is what some might think of as a state vector.

The name that I think is probably most accurate and least likely to be confused with some other entity in this arena is

getVectorOfStateObjects()

since an std::vector<> is returned.

If you agree, I'll go ahead and change the method name to getVectorOfStateObjects().

Agreed

[fcanderson]

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

Previously, aymanhab (Ayman Habib) wrote…

Amazingly clear description @fcanderson Thank you 🥇

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

[aymanhab]

Very close, I believe I responded to your questions, let's address the remaining items to get it over the hump. Thank you

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @fcanderson)

---

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

Previously, fcanderson (Clay Anderson) wrote…

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?

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.

---

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

Previously, fcanderson (Clay Anderson) wrote…

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

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.

[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.

[aymanhab]

Thanks @fcanderson and I agree completely with your assessment regarding the holes in the current system before this PR, and appreciate the work you've done to enable this important functionality.

My question was regarding the StateTrjectory class/methods e.g. exportToTable has no way to tell where it came from, could it export discrete variables or modeling-options? Maybe it doesn't matter (that's a totally satisfactory resolution) or if not we could maintain a flag for book-keeping. Mainly asking for clarification for users who are already utilizing the StatesTrajectory vehicle elsewhere. If that's a more involved discussion or something that you resolved earlier or better done on Zoom I'm totally open. Thanks again.

Reviewed 4 of 4 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @fcanderson)

[fcanderson]

My understanding is that StatesTrajectory generally gets the states from a StatesTrajectoryReporter, in which case the trajectory of SimTK::State objects it contains will have valid values for all state categories (continuous, discrete, modeling). If, however, StatesTrajectory gets its states from a saved file, then any instance of a SimTK::State that is based only on reading that file will only have valid values for the Continuous states; the discrete variables and modeling options will be left uninitialized. Valid values of discrete and modeling states, in this case, will have to come from another source (e.g., another input file).

A flag in the StatesTrajectory to indicate whether it contains an entirely valid trajectory of SimTK::State objects seems like it could be a useful thing. If the origin of the trajectory was from a StatesTrajectoryReport, the flag is set to COMPLETE. On the other hand, if the origin of the trajectory was from file, then the flag is set to INCOMPLETE.

In its current form, StatesTrajectory doesn't export discrete variables or modeling options. It could be modified to do so, however. OpenSim::Component now has the infrastructure to efficiently get or set each state variable (continuous, discrete, or modeling) individually from or into a SimTK::Array_SimTK::State by specifying the variable's path. OpenSim::Component keeps an std::map for each state category using path as the hash index.

Enhancing StatesTrajectory would be a natural step to take except for one road block (that I am aware of). TimeSeriesTable does not currently have a way to co-mingle variables of different types. I really don't like the idea of serializing to multiple files, one for each type encountered (one for the int's, one for double's, one for Vec3's, etc.). That seems like a recipe for a logistical nightmare. How do you manage to keep track of all the files that go together?

The appeal of using XML, at least as for an initial pass at this functionality, is that the xml attribute mechanism was a really natural way to encode variable type. AND, SimTK::Xml already knows how to serialize and deserialize ALL possible variable types that could be encountered in Simbody (matrices, SpatialVec, etc). Using XML presented a clear path to getting all states in ONE file with a model name, appropriate precision, time stamp, and note.

If .ostate file size becomes an issue, I do think pursuing binary serialization would be a productive way to go. To get this functionality, augmenting the appropriate Simbody classes I think should be the point of attack. There would just need to be a flag that is set in SimTK::Xml to signal binary serialization, and each concrete SimTK::Value type would need to know how to stream itself in binary form. The functionality may actually already exist. I haven't looked into it.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @fcanderson)

[fcanderson]

A follow-up:

1. I read about binary XML formats. It appears like a much debated topic. Most people, it seems, are not in favor of binary because it removes one of the hallmark design characteristics of XML -- it is user readable and editable with common editors.
2. A good alternative to using some kind of binary XML to reduce file size is using compression (e.g., zip'ing).
3. In StatesTrajectory.h, I do make a disclaimer about using StatesTrajectory::createFromStorage() and createfromStatesTable() as a deserialization approach.
   See https://github.com/fcanderson/opensim-core/blob/ostates_2/OpenSim/Simulation/StatesTrajectory.h#L314

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @fcanderson)

[fcanderson]

Reviewable status: 8 of 10 files reviewed, 1 unresolved discussion (waiting on @aymanhab)

---

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

Previously, fcanderson (Clay Anderson) wrote…

Working on it.

Done.

[fcanderson]

@aymanhab I believe I've addressed all the requested changes.

You'll notice that I pushed 1 or 2 more commits. I took the liberty of adjusting the documentation to reflect that both std::vecter<SimTK::State> or SimTK::Array_<SimTK::State> will work as in interface to class StatesDocument.

If all looks good to you, then it looks good to me.

Reviewable status: 8 of 10 files reviewed, 1 unresolved discussion (waiting on @aymanhab)

[aymanhab]

Thanks @fcanderson Looks great. Please update the changelog and we're done. Great work 👍

Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @fcanderson)

[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.

[aymanhab]

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @fcanderson)