Add rotateMarkerTable and tests

[alexbeattie42]

Fixes issue #4146

Brief summary of changes

• Add rotateMarkerTable and tests

Testing I've completed

• Added unit tests
• Tested locally with my dataset

Looking for feedback on...

@nickbianco can you take a look?

CHANGELOG.md (choose one)

• updated.

---

This change is 

[nickbianco]

Thanks @alexbeattie42! Looks good overall. I left a handful of mostly minor comments about formatting and style.

This made me wonder if it would be better to add a method specific to the TimeSeriesTable_<Vec3> specialization (see this SO post), something like TimeSeriesTableVec3::rotate(const SimTK::Rotation&).

@nickbianco reviewed 4 files and all commit messages, and made 7 comments.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on alexbeattie42).

---

CHANGELOG.md line 45 at r1 (raw file):

- Fixed an issue prevent element-by-element construction of `Mat33` objects in Matlab and Python. (#4241)
- Added class `MeyerFregly2016Muscle` to support NMSM Pipeline-equivalent muscle models in Moco. (#4233)
- Added `rotateMarkerTable` utility method. (#4146)

Could update this to state where the utility was added, i.e., CommonUtilities?

---

OpenSim/Common/CommonUtilities.h line 277 at r1 (raw file):

 *
 * @return: None
 */

Some nit-picky suggestions on the Doxygen comments here (punctuation, full sentences, etc):

Suggestion:

/**
 * Rotate all elements of a TimeSeriesTableVec3 (e.g., marker positions) by a 
 * specified SimTK::Rotation.
 *
 *
 * @param table the table of Vec3 elements to be modified in place.
 * @param rotation the SimTK::Rotation representing the desired rotation.
 */

---

OpenSim/Common/CommonUtilities.h line 281 at r1 (raw file):

OSIMCOMMON_API
void rotateMarkerTable(
        OpenSim::TimeSeriesTableVec3& table, const SimTK::Rotation& R);

While this might be most commonly be used for rotating tables of marker positions, it could be used for any TimeSeriesTableVec3.

Also, I would flip the order of arguments (since the table is modified) and you could also omit the OpenSim namespace:

Suggestion:

OSIMCOMMON_API
void rotateTimeSeriesTableVec3(const SimTK::Rotation& table, TimeSeriesTableVec3& rotation);

---

OpenSim/Common/Test/testCommonUtilities.cpp line 322 at r1 (raw file):

        markerData(2, 0) = SimTK::Vec3(7, 8, 9);

        OpenSim::TimeSeriesTableVec3 table(time, markerData, labels);

Could omit the OpenSim:: namespace here and elsewhere in this file.

---

OpenSim/Common/Test/testCommonUtilities.cpp line 344 at r1 (raw file):

        for (int i = 0; i < A.nrow(); ++i)
            for (int j = 0; j < A.ncol(); ++j)
                REQUIRE(A(i, j).isNumericallyEqual(B(i, j)));

Perhaps my personal preference that defined OpenSim style, but consider adding braces for this nested for-loop for readability.

---

OpenSim/Common/Test/testCommonUtilities.cpp line 378 at r1 (raw file):

        for (int i = 0; i < A.nrow(); ++i)
            for (int j = 0; j < A.ncol(); ++j)
                REQUIRE(A(i, j).isNumericallyEqual(B(i, j)));

See comment above.

[alexbeattie42]

@nickbianco Thanks for the review! I updated the PR and moved the method to be a specialization of TimeSeriesTableVec3

@alexbeattie42 made 7 comments.
Reviewable status: 0 of 5 files reviewed, 6 unresolved discussions (waiting on nickbianco).

---

CHANGELOG.md line 45 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Could update this to state where the utility was added, i.e., CommonUtilities?

Done.

---

OpenSim/Common/CommonUtilities.h line 277 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Some nit-picky suggestions on the Doxygen comments here (punctuation, full sentences, etc):

Done.

---

OpenSim/Common/CommonUtilities.h line 281 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

While this might be most commonly be used for rotating tables of marker positions, it could be used for any TimeSeriesTableVec3.

Also, I would flip the order of arguments (since the table is modified) and you could also omit the OpenSim namespace:

Done.

---

OpenSim/Common/Test/testCommonUtilities.cpp line 322 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Could omit the OpenSim:: namespace here and elsewhere in this file.

Done.

---

OpenSim/Common/Test/testCommonUtilities.cpp line 344 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Perhaps my personal preference that defined OpenSim style, but consider adding braces for this nested for-loop for readability.

Done.

---

OpenSim/Common/Test/testCommonUtilities.cpp line 378 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

See comment above.

Done.

[adamkewley]

Nice contribution and the tests are highly appreciated. My only comment is that the implementation code could be simpler.

rotate is also an unusual name. Remember, it's a method on the table. Do you want to rotate the table or do you want to rotate each cell in the table? Isnt rotation a specialization of affine transformation? And isnt affine transformation a specialization of data transformation? Given that, why not: Table::forEachCell(const std::function<void(T&)>&)? That way, any caller can apply any transformation they want - and you don't need a custom iterator.

I've also suggested a higher-level change: kick it downstream by making data access easier (via a custom iterator). That way, the caller's code is more readable, and is compatible with standard algorithms, and the callee's code doesn't need to be littered with alsorts of specialized functions because someone found it annoying to iterate over a table at some point.

@adamkewley reviewed 5 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on alexbeattie42 and nickbianco).

---

OpenSim/Common/TimeSeriesTable.h line 577 at r2 (raw file):

                std::transform(row.begin(), row.end(), row.begin(),
                        [&R](const SimTK::Vec<3>& v) { return R * v; });
            });

@nickbianco and @alexbeattie42 . Great contribution/review, but I think you both need to take a step back and focus on the implementation/architecture.

It's over-the-top to zero-allocate a vector, iota-populate it, then use std::for_each and std::transform in order to implement:

for (int row = 0, nRows = static_cast<int>(getNumRows()); row < nRows; ++row) {
    for (auto& cell : _depData.updRow(row)) {
        cell = R*cell;
    }
}

Bigger picture (not necessary for this PR), I'd go as far as arguing that what's actually necessary from an architectural PoV is a .cells() iterator. It would mean that template-specialized functions like this aren't necessary and callers can write what they need downstream:

for (auto& cell : _depData.cells()) { cell = R*cell; }

// If you're still hell-bent on algorithms...
std::ranges::for_each(_depData.cells(), [&R](auto& cell) { cell = R*cell; });

No documentation or naming convention necessary: anyone with an understanding of C++ could guess what that code is up to, and could specialize it downstream (e.g. add offsets, subtract origins, re-express the vectors, etc. etc.).

[nickbianco]

Thanks for the input @adamkewley. On second look, I agree that rotate is ambiguous here and that it is a bit messy to tie this utility to TimesSeriesTable as a static method. I think it makes sense to keep this isolated as a utility until something like the suggested cells() iterator approach is available down the road.

@alexbeattie42 I would say let's move this back to CommonUtilities (sorry for the flip-flop) with a new implementation similar to Adam's suggestion below, i.e., just modify rows in place without allocating any new vectors. We should also come up with an unambiguous name, perhaps rotateTimeSeriesTableVec3Elements or something similar.

@nickbianco reviewed 2 files and all commit messages, made 2 comments, and resolved 6 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on alexbeattie42).

---

OpenSim/Common/TimeSeriesTable.h line 577 at r2 (raw file):

Previously, adamkewley (Adam Kewley) wrote…

@nickbianco and @alexbeattie42 . Great contribution/review, but I think you both need to take a step back and focus on the implementation/architecture.

It's over-the-top to zero-allocate a vector, iota-populate it, then use std::for_each and std::transform in order to implement:

for (int row = 0, nRows = static_cast<int>(getNumRows()); row < nRows; ++row) {
    for (auto& cell : _depData.updRow(row)) {
        cell = R*cell;
    }
}

Bigger picture (not necessary for this PR), I'd go as far as arguing that what's actually necessary from an architectural PoV is a .cells() iterator. It would mean that template-specialized functions like this aren't necessary and callers can write what they need downstream:

for (auto& cell : _depData.cells()) { cell = R*cell; }

// If you're still hell-bent on algorithms...
std::ranges::for_each(_depData.cells(), [&R](auto& cell) { cell = R*cell; });

No documentation or naming convention necessary: anyone with an understanding of C++ could guess what that code is up to, and could specialize it downstream (e.g. add offsets, subtract origins, re-express the vectors, etc. etc.).

I totally agree about not needing the extra vector allocation here, thanks for raising that point.

A .cells() iterator would be great! Do you know if an iterator like this works with Python bindings? It would be a shame to unify methods for modifying table elements only to need utilities/wrappers to access them in scripting.

[adamkewley]

You could even make the naming unambiguous by slightly generalizing the operation to take a const SimTK::Transform&, or call the method transformCellValues(Table&, const SimTK::Rotation&). One could imagine overloads being added as-needed over time:

• transformCellValues(Table&, const SimTK::Rotation&)
• transformCellValues(Table&, const SimTK::Transform&)
• transformCellValues(Table&, const SimTK::Mat4&)

Although, saying that, it reveals a mathematical ambiguity: are the values points/stations or vectors? Simbody would usually separate them:

• transformCellsAsVectors(Table&, const SimTK::Transform&)
• transformCellsAsStations(Table&, const SimTK::Transform&) // usually called "points", but "stations" is OpenSim/Simbody naming

And now you're seeing why designing APIs properly is rough ;)

@adamkewley made 3 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on alexbeattie42).

---

OpenSim/Common/TimeSeriesTable.h line 577 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

I totally agree about not needing the extra vector allocation here, thanks for raising that point.

A .cells() iterator would be great! Do you know if an iterator like this works with Python bindings? It would be a shame to unify methods for modifying table elements only to need utilities/wrappers to access them in scripting.

I don't know how SWIG talks to iterators, but I imagine the ComponentListIterator<T> would have to do this already, if you need inspiration.

However, the tradeoff for scripting is performance vs. usability. Iterators are fine in C++ but require hopping in-and-out of the scripting context via bindings, which can become extremely expensive. In that case, having specialized methods like what's suggested here becomes useful again because it enables doing all of the computation in C++ from a single scripting call site.

---

OpenSim/Common/TimeSeriesTable.h line 514 at r3 (raw file):

    }
    void rotate(const SimTK::Rotation& R) {
        OPENSIM_THROW(Exception,

This is the issue with adding a template specialization as a member method with aliases like using TimeSeriesTableVec3 = TimeSeriesTable<Vec3>;: you now need runtime checks in the base template. The C++ compiler could know perfectly whether a particular table type does or doesn't have this specialization - a runtime check is messy.

I imagine the only way around this, if you're desperate for a member method, would be to create a new type called TimeSeriesTableVec3 via inheritance rather than aliasing. The new type could have its own methods (e.g. rotate`) but, of course, you have all the issues of inherited methods not returning the same type (there's no easy victories here). That's where patterns like CRTP creep in.

Alternatively, having a free (non-member) function like void transformCellsAsVectors(TimeSeriesTableVec3&, const SimTK::Rotation&) doesn't have those requirements because the type isn't being extended with new member methods (it's a separate function).

[alexbeattie42]

Thanks for the feedback and review @adamkewley and @nickbianco!
I'm going to close this for now in favor of figuring out a proper solution in: simbody/simbody#848.

I can use the shims I have been using in my own code for now until a more proper solution is developed.