Speedup testCMCSingleMuscle

[aseth1]

Shorten and improve single muscle CMC test, which compares the results from CMC when tracking kinematics of a forward simulation using the same model. The model file version was updated to 3.0 (since test developed in 3.3) and the duration was shortened (5s->0.9s) and the peek excitation was doubled. Movement is more dynamic (faster and more range) and the controls and output forces are now being compared as well as states.

In addition to being a harder test, this should also be at least 5x faster than the previous test case.
On Appveyor 32bit: ~180s to ~14s

[tkuchida]

👍 LGTM. This is a great way to speed up the tests (some were probably designed years ago when we relied on nightly builds, where speed wasn't a concern). However, I think tests like this should use models generated programmatically rather than storing and maintaining model files; separate tests should exercise model file loading and compare rigorously to ensure all properties are read in correctly, for example.

[aseth1]

I think tests like this should use models generated programmatically rather than storing and maintaining model files; separate tests should exercise model file loading and compare rigorously to ensure all properties are read in correctly

I agree. In this case building the model (programmatically) would be straightforward but setting up the CMCTool could be troublesome, since the Tools were never designed to be called directly from the API. I would prefer not to convert this test, but require that future tests (e.g. for a replacement for CMC and new solvers) have separate tests for performance, implementation correctness and de/serialization.

Moving forward with new components, testing serialization, cloning, connectors and availability of outputs, can be tested by a generic set of cases as part of testComponents. So we can continue to reduce the dependency on setup and model files for testing core functionality as more functionality is encompassed by Components. As for Tools, I think it is just easier to include the setup files until we can deprecate them altogether (after 4.0), but that could also be a long time.

I could also attempt to setup the Forward and CMC Tools programmatically for this test. What is your preference for this PR?

[tkuchida]

What is your preference for this PR?

No need to hold up this PR. I suggest opening a new issue for (1) generating models (and setup files, wherever possible) programmatically rather than loading them from xml, and (2) creating a new test focusing on serialization/deserialization that includes loading models saved in each supported version.

[aseth1]

Sounds good.

(2) creating a new test focusing on serialization/deserialization that includes loading models saved in each supported version.

There are existing tests, in particular, testSerializeOpenSimObjects and testComponents that test serialization and deserialization and equality of models. However, testSerializeOpenSimObjects appears to be limited to ModelComponents which makes it far from being complete for testing OpenSim "Objects". I added an issue #1150 about this. There is an open issue for model conversion testing, #206.

(1) generating models (and setup files, wherever possible) programmatically rather than loading them from xml

I opened an issue, #1151, to propose adding a new guideline to our CONTRIBUTING.md to avoid the use of files (model or otherwise) for unit and functional testing (unless of course its function is to read/write data) and promote the use of short integration and regression tests.

If this is satisfactory please feel free to merge.

[tkuchida]

If this is satisfactory please feel free to merge.

LGTM. This PR exceeds what is covered by Rule 5; by Rule 6, a second reviewer needs to merge.

[aseth1]

@klshrinidhi please give this a quick review when you have the opportunity. Thanks.

[klshrinidhi]

If possible, it would be good to not add any more code that uses OpenSim::Array and use std::vector instead.

Other than that, I don't see any problem. 👍

[aseth1]

Thanks @klshrinidhi. I agree with your request. The added OpenSim::Arrays are used to define the tolerances as expected by CHECK_STORAGE_AGAINST_STANDARD() and updating that method would impact numerous test cases. Making that update perhaps should wait until we are closer to eliminating OpenSim::Array altogether. Otherwise, if we decide to back OpenSim::Array with std::vector, this might not be an issue either. A good discussion perhaps for a future PR?

[aseth1]

@tkuchida @klshrinidhi merge?

[tkuchida]

A good discussion perhaps for a future PR?

Have you created an issue, or is it covered by an existing one?

[aseth1]

@tkuchida I just added #1194