Support analog data read by ezc3d

[aymanhab]

Fixes issue #3083 #2575

Brief summary of changes

Beef up C3DFileAdapter to return a table of AnalogData as TimeSeriesTable

Testing I've completed

Visually inspected analogData file and data looked to match the grf file. Also tested by @nicos1993

Looking for feedback on...

CHANGELOG.md (choose one)

• updated.

---

This change is 

[aymanhab]

@nicos1993 This seems to be workig now for the sample files you sent me. Would be good to test the functionality manually while I handle the corner cases and beef up the test case. Ideally you have a small .c3d file with actual data that we can put under source control for the test. Thanks

[nicos1993]

@aymanhab Hello, I did a little further testing. It seems to be working as anticipated (pulling ALL analogue signals), users will have to be careful to ignore the ground reaction force data extracted from getAnalogDataTable as it does not undergo the conversions necessary for usage - as is performed in getForcesTable

I created an analog_table std::shared_ptr<TimeSeriesTable> analog_table = c3dFileAdapter.getAnalogDataTable(tables);
Extracted column labels std::vector<std::string> labels1 = analog_table->getColumnLabels();
Wrote the analog_table using STOFileAdapter STOFileAdapter sto_adapter_1{};
sto_adapter_1.write(*analog_table, "test.sto");

In summary, the test.sto contains all the analog data as expected and I should be able to operate on it easily!

Nicos

[carmichaelong]

Thanks @aymanhab, looking good overall, just a few minor comments. Also up to you if you want to expand the scope of this PR or defer to another one (if the latter, just make sure to start a new issue for tracking)

Reviewed 2 of 2 files at r1, 4 of 5 files at r3.
Reviewable status: 6 of 7 files reviewed, 4 unresolved discussions (waiting on @aymanhab)

---

CHANGELOG.md line 34 at r3 (raw file):

- Fixed out-of-bounds memory access in testAssemblySolver (#3460)
- Fixed runtime segfault that can occur when trying to use a `WrapObject` that is not a child of a `PhysicalFrame` (#3465)
- Fixed issues #3083 #2575 where analog data is not pulled out from c3d files, a getAnalogDataTable() has been implemented and added to the C3DFileAdapter

typo? "c3d files, a getAnalogDataTable()" -> "c3d files, a new function get AnalogDataTable()"

---

OpenSim/Common/C3DFileAdapter.cpp line 349 at r3 (raw file):

            }
            analog_data_matrix.updRow(rowNumber) = row;
            analog_times[rowNumber] = rowNumber * analog_time_step; //TODO: 0 should be start_time

can you explain this TODO more? is it that this assumes that the analog data always starts at time = 0? do c3d files generally allow for files that start at a non-zero time?

---

OpenSim/Common/Test/testC3DFileAdapter.cpp line 108 at r3 (raw file):

    downsample_table(*marker_table, 10);
    downsample_table(*force_table, 100);
    // analog table has same data as forces table but 

it's not clear to me why a different frame would need more downsampling. are these two separate comments? also looks like from the test lines below that the analog table is tested against the analog file (and not the forces table or file)

---

OpenSim/Common/Test/testStorage.cpp line 188 at r3 (raw file):

        SimTK_SUBTEST2(testStorageLoadingFromFile, "dataWithNaNsOfDifferentCases.trc", 43);

        #if defined (WITH_EZC3D)

it's unclear to me why this test was removed. no need to add a comment into the code, responding to this comment is sufficient!

[aymanhab]

Thanks for the feedback @carmichaelong, I addressed your comments but please let me know if you think more clarification is needed.

Reviewable status: 6 of 7 files reviewed, 3 unresolved discussions (waiting on @carmichaelong)

---

OpenSim/Common/C3DFileAdapter.cpp line 349 at r3 (raw file):

Previously, carmichaelong wrote…

can you explain this TODO more? is it that this assumes that the analog data always starts at time = 0? do c3d files generally allow for files that start at a non-zero time?

This follows the exact same TODO for forces, basically there's no indication in the file regarding start time, only sampling frequency so both forces and analog data assume start at 0

---

OpenSim/Common/Test/testC3DFileAdapter.cpp line 108 at r3 (raw file):

Previously, carmichaelong wrote…

it's not clear to me why a different frame would need more downsampling. are these two separate comments? also looks like from the test lines below that the analog table is tested against the analog file (and not the forces table or file)

These test .c3d files have only forces data, and no other analog data. So the test for contents is already done. I visually compared the numbers in the few rows of the highly downsampled analog vs forces files and they checked. There's no need to add another duplicate analog data file to the repo.

---

OpenSim/Common/Test/testStorage.cpp line 188 at r3 (raw file):

Previously, carmichaelong wrote…

it's unclear to me why this test was removed. no need to add a comment into the code, responding to this comment is sufficient!

This test was constructing a storage from c3d file and comparing against a specific table even though multiple tables are returned. With analogData table added, a different table is now returned and so the test failed. This case is just a bad test since we should never promise to randomly pick a specific table when c3d contains multiple tables. I don't think users should count on this behavior.

[carmichaelong]

thanks @aymanhab the clarifications were all super helpful. i left one comment that could be ignored depending on the bang for buck of adding a test to check for just a few values

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

---

OpenSim/Common/Test/testC3DFileAdapter.cpp line 108 at r3 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

These test .c3d files have only forces data, and no other analog data. So the test for contents is already done. I visually compared the numbers in the few rows of the highly downsampled analog vs forces files and they checked. There's no need to add another duplicate analog data file to the repo.

thanks that's helpful for context, and i fully agree that we shouldn't add another duplicate file to the repo.

seems like it could still be helpful if there were even a few numbers that could be checked between the analog and forces files (e.g., like you did visually, and could be as simple as a few values in the first few rows). i know this might be tricky with all the downsampling, so this can be ignored.

[aymanhab]

@carmichaelong for your review

[carmichaelong]

thanks @aymanhab, just a couple typos in the changelog and comments. feel free to fix those without CI and merge

Reviewed 1 of 1 files at r4, 1 of 1 files at r5.
Reviewable status: all files reviewed (commit messages unreviewed), 3 unresolved discussions (waiting on @aymanhab)

---

CHANGELOG.md line 34 at r5 (raw file):

- Fixed out-of-bounds memory access in testAssemblySolver (#3460)
- Fixed runtime segfault that can occur when trying to use a `WrapObject` that is not a child of a `PhysicalFrame` (#3465)
- Fixed issues #3083 #2575 where analog data is not pulled out from c3d files, a a new function getAnalogDataTable() has been added to the C3DFileAdapter

there still seems to be a typo in here "a a new function..."

---

OpenSim/Common/Test/testC3DFileAdapter.cpp line 205 at r5 (raw file):

    cout << "\t" << analogs_file << " is equivalent to its standard." << endl;

    // For the test files, analog data contains force vectors with some rogod body transform/signs changed

rogod -> rigid

[aymanhab]

Fixed typos, thanks @carmichaelong and @nicos1993

[pariterre]

@aymanhab
Glad to see it moved quickly forward! May I suggest to update ezc3d to 1.5.4 in the dependencies. There should not be any change that affects you in the API, I however had a bug in the Matlab binder for the 1.5.0 which causes problem for those who have OpenSim installed (and try to use ezc3d on Matlab) :)

[aymanhab]

DEfinitely, and thanks for all your help and for making it so easy to implement on our side. I'll open an issue to put in our next sprint in a couple weeks.