Add TableUtilities and more CommonUtilities. #2808
Conversation
Update VisualizerUtilities::showMotion() and StatesTrajectory to use a TimeSeriesTable.
|
Tests finally pass :) |
|
Nothing particularly unusual in there. Most of the stuff I spotted was more "things to think about", rather than game-changers, so I'd just take what I've written non-seriously if you need to ship this quickly. |
There was a problem hiding this comment.
Reviewed 1 of 28 files at r1.
Reviewable status: 1 of 28 files reviewed, 1 unresolved discussion (waiting on @aymanhab, @carmichaelong, and @chrisdembia)
CHANGELOG.md, line 18 at r1 (raw file):
- Whitespace is trimmed when reading table metadata for STO, MOT, and CSV files. - Introduce the following utility functions: - make_unique()
Where? Just reading this in release notes without context, users have no clue. Do we need this itemized list in CHANGLEOG or just a short summary, maybe an issue with details.
There was a problem hiding this comment.
Reviewable status: 1 of 28 files reviewed, 1 unresolved discussion (waiting on @aymanhab and @carmichaelong)
CHANGELOG.md, line 18 at r1 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
Where? Just reading this in release notes without context, users have no clue. Do we need this itemized list in CHANGLEOG or just a short summary, maybe an issue with details.
@aymanhab let me know which of the two approaches edits you prefer:
Introduce the following utility functions in OpenSim/Common/CommonUtilities.h:
versus
Introduce utilities for creating SimTK::Vectors, linear interpolation, updating table column labels from v3.3 to v4.0 syntax, and solving for a function's root using bisection (OpenSim/Common/CommonUtilities.h).
I prefer the second which gives the high level functionality as seen by users. We can link to the PR or an issue summarizing the change if some user wants to go deeper. Thanks @chrisdembia |
Done. |
There was a problem hiding this comment.
Reviewed 7 of 28 files at r1, 1 of 1 files at r2.
Reviewable status: 6 of 27 files reviewed, 4 unresolved discussions (waiting on @aymanhab, @carmichaelong, and @chrisdembia)
OpenSim/Common/CommonUtilities.h, line 75 at r1 (raw file):
OSIMCOMMON_API SimTK::Vector createVectorLinspace(int length, double start, double end);
Linspace is a bit unclear, uniformSpace? Are there similar matlab functions that users maybe familiar with?
OpenSim/Common/CommonUtilities.h, line 99 at r2 (raw file):
const std::string& documentFileName, const std::string& pathnameRelativeToDocument);
The method name is a bit ambiguous because XML has a native PATH concept (sequence of nodes). The name could be something like convertDocumentRelativePathnameToAbsolute() or convertRelativeFilePathToAbsolute(). Thoughts?
OpenSim/Common/CommonUtilities.h, line 103 at r2 (raw file):
/// If the values of calcResidual(left) and calcResidual(right) have the same /// sign and Logger::shouldLog(Logger::Level::Debug), then /// this function writes a file `solveBisection_residual_<timestamp>.sto`
I'd use Logger level set to Debug, and not hardcode the actual signature of shouldLog since there are multiple ways to set it.
There was a problem hiding this comment.
Reviewed 4 of 28 files at r1, 1 of 4 files at r3.
Reviewable status: 11 of 27 files reviewed, 7 unresolved discussions (waiting on @aymanhab, @carmichaelong, and @chrisdembia)
OpenSim/Common/CommonUtilities.cpp, line 120 at r3 (raw file):
const auto& newXi = newX[i]; if (x_no_nans[0] <= newXi && newXi <= x_no_nans[x_no_nans.size() - 1]) newY[i] = function.calcValue(SimTK::Vector(1, newXi));
Not sure what assumptions are made regarding x monotonically increasing (doesn't seem to be enforced) are newXi guaranteed by construction to fall between the x_no_nans bounds?
OpenSim/Common/CommonUtilities.cpp, line 145 at r3 (raw file):
OPENSIM_THROW_IF(maxIterations < 0, Exception, "Expected maxIterations to be positive, but got {}.",
So 0 is allowed? Is this intentional?
OpenSim/Common/CommonUtilities.cpp, line 157 at r3 (raw file):
row[0] = calcResidual(x[i]); table.appendRow(x[i], row); }
appendRow is known to be terrible time consuming call since it copies the table contents to add a new row, if time critical I'd avoid it
OpenSim/Common/IO.h, line 129 at r3 (raw file):
/// https://stackoverflow.com/questions/874134/find-if-string-ends-with-another-string-in-c static bool EndsWith(const std::string& string, const std::string& ending); static void eraseEmptyElements(std::vector<std::string>& list);
Typically this methods have ignore case option or variant
There was a problem hiding this comment.
Reviewed 4 of 28 files at r1, 1 of 4 files at r3.
Reviewable status: 16 of 27 files reviewed, 8 unresolved discussions (waiting on @aymanhab, @carmichaelong, and @chrisdembia)
OpenSim/Simulation/Test/testStatesTrajectory.cpp, line 512 at r3 (raw file):
itime++; } }
Is this tested somewhere else?
There was a problem hiding this comment.
Reviewed 2 of 28 files at r1.
Reviewable status: 18 of 27 files reviewed, 10 unresolved discussions (waiting on @aymanhab, @carmichaelong, and @chrisdembia)
OpenSim/Common/Signal.h, line 90 at r3 (raw file):
static std::vector<double> Pad(int aPad, int aN, const double aSignal[]); static void Pad(int aPad, OpenSim::Array<double>& aSignal);
Is there a reason to keep around 2 versions of Pad with 2 different interfaces and datatypes? Can these be unified?
OpenSim/Common/TimeSeriesTable.h, line 515 at r3 (raw file):
friend class TableUtilities;
Why do we need this friendship, if TableUtilities is using the public interface of TimeSeriesTable
There was a problem hiding this comment.
Reviewed 8 of 28 files at r1, 1 of 4 files at r3.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @carmichaelong and @chrisdembia)
OpenSim/Common/Test/testDataTable.cpp, line 915 at r3 (raw file):
CHECK(column[4] == Approx(0.2)); CHECK(column[5] == Approx(0.0).margin(1e-10)); }
What's the use of margin and why only in this case/column?
OpenSim/Simulation/SimulationUtilities.h, line 59 at r3 (raw file):
/// This can also be used to update the column labels of an Inverse /// Kinematics Tool solution MOT file so that the data can be used as /// states. If a label does not identify a state in the model, the column
MOT files are typically in degrees and don't contain muscle states, that's fine for a lower level utility but maybe some warning if states are missing would help
OpenSim/Simulation/StatesTrajectory.h, line 449 at r3 (raw file):
* import opensim * model = opensim.Model("subject01.osim") * table = opensim.TimeSeriesTable("subject01_states.sto")
While useful to have the example here, this tends to be brittle since they need to be maintained, wondering if there's a better more sustainable solution. If we upgrade SWIG we may not need these altogether, I think
|
Finished a pass of review, overall looks good, left a few questions/suggestions. Thanks @chrisdembia |
There was a problem hiding this comment.
Ready for review.
Reviewable status: 22 of 28 files reviewed, 13 unresolved discussions (waiting on @aymanhab and @carmichaelong)
CHANGELOG.md, line 18 at r1 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
I prefer the second which gives the high level functionality as seen by users. We can link to the PR or an issue summarizing the change if some user wants to go deeper. Thanks @chrisdembia
Done.
OpenSim/Common/CommonUtilities.h, line 75 at r1 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
Linspace is a bit unclear, uniformSpace? Are there similar matlab functions that users maybe familiar with?
We chose this name because it's familiar to Matlab users: http://www.ece.northwestern.edu/local-apps/matlabhelp/techdoc/ref/linspace.html
OpenSim/Common/CommonUtilities.h, line 99 at r2 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
The method name is a bit ambiguous because XML has a native PATH concept (sequence of nodes). The name could be something like convertDocumentRelativePathnameToAbsolute() or convertRelativeFilePathToAbsolute(). Thoughts?
Ah good point. I attempted a new name.
OpenSim/Common/CommonUtilities.h, line 103 at r2 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
I'd use Logger level set to Debug, and not hardcode the actual signature of shouldLog since there are multiple ways to set it.
Done.
OpenSim/Common/CommonUtilities.cpp, line 120 at r3 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
Not sure what assumptions are made regarding x monotonically increasing (doesn't seem to be enforced) are newXi guaranteed by construction to fall between the x_no_nans bounds?
I just added such a check to the constructor of PiecewiseLinearFunction.
OpenSim/Common/CommonUtilities.cpp, line 145 at r3 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
So 0 is allowed? Is this intentional?
Yes, 0 is allowed.
In Moco, we use Ipopt and it permits 0 iterations.
OpenSim/Common/CommonUtilities.cpp, line 157 at r3 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
appendRow is known to be terrible time consuming call since it copies the table contents to add a new row, if time critical I'd avoid it
Done.
OpenSim/Common/IO.h, line 129 at r3 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
Typically this methods have ignore case option or variant
Done.
OpenSim/Common/Signal.h, line 90 at r3 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
Is there a reason to keep around 2 versions of Pad with 2 different interfaces and datatypes? Can these be unified?
I've unified the implementations but left both interfaces.
OpenSim/Common/TimeSeriesTable.h, line 515 at r3 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
Why do we need this friendship, if TableUtilities is using the public interface of TimeSeriesTable
The pad utility modifies the table's _indData, which is not public :(
OpenSim/Common/Test/testDataTable.cpp, line 915 at r3 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
What's the use of margin and why only in this case/column?
Margin is a permitted absolute error; it's used here because Approx(0.0) is not sufficient on its own: catchorg/Catch2#1444
OpenSim/Simulation/SimulationUtilities.h, line 59 at r3 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
MOT files are typically in degrees and don't contain muscle states, that's fine for a lower level utility but maybe some warning if states are missing would help
We use this in Moco when we track kinematics from an IK solution, in which case we need to update the column labels but it's fine that muscle states are missing.
OpenSim/Simulation/StatesTrajectory.h, line 449 at r3 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
While useful to have the example here, this tends to be brittle since they need to be maintained, wondering if there's a better more sustainable solution. If we upgrade SWIG we may not need these altogether, I think
I think it's easiest to maintain documentation that resides in the repository (rather than separately editing Confluence).
I think these examples are still useful even if we move to SWIG 4, because then Python users can access this Python example within their Python interpreter.
The best documentation is example code; I've updated posthoc_StatesTrajectory_example.py in this PR.
OpenSim/Simulation/Test/testStatesTrajectory.cpp, line 512 at r3 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
Is this tested somewhere else?
No, I've removed this check. For TimeSeriesTables, all rows have the same length. I think this check was largely unnecessary anyway.
There was a problem hiding this comment.
Reviewed 7 of 7 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aymanhab and @carmichaelong)
|
Thanks @aymanhab |
Part of #2559
Brief summary of changes
Some of these changes are required for #2793 (e.g.,
solveBisection()).Testing I've completed
Looking for feedback on...
naming of new functions.
CHANGELOG.md (choose one)
@carmichaelong @aymanhab I don't think we need both of you to review, but either of you could review, so I've added both of you as reviewers.
@adamkewley , no pressure whatsoever, but feel free to pitch in on this review if you're interested. @aymanhab and @carmichaelong already have some context for this PR, so if you want more context, we can chat about it.
This change is