Add utility to append two TimeSeriesTable(s) and produce a new one.

[aymanhab]

Fixes issue #0

Brief summary of changes

Added utility to concatenate two tables and create a new one, useful for stitching segments

Testing I've completed

Looking for feedback on...

CHANGELOG.md (choose one)

• no need to update because...
• updated.

---

This change is 

[aymanhab]

@mattpetrucci Please let me know if the interface looks usable and/or the naming is good enough as is.

[aymanhab]

Here's how it's used successfully in the GUI side to stitch trc and mot files
https://github.com/opensim-org/opensim-visualizer/blob/de036792312b9512add94a63dae49c414a78bb67/jxbrowser/src/org/opensim/addBiomech/AddBiomechanicsTrial.java#L100

[nickbianco]

Nice work @aymanhab! I've left a handful of comments, mostly related to formatting. I do think we should consider using the name TimeSeriesTable::concatenate() to match the naming convention of other table utilities.

@nickbianco reviewed 4 files and all commit messages, and made 16 comments.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on aymanhab).

---

-- commits line 4 at r1:
Like we've been doing recently, let's squash all commits into one before merging.

---

Bindings/common.i line 393 at r1 (raw file):

%template(concatenateTableVec3) OpenSim::TableUtilities::concatenateTable<SimTK::Vec3>;
%template(concatenateTable) OpenSim::TableUtilities::concatenateTable<double>;

Would it make sense to add other template instantiations here, e.g., Rotation, Quaternion, while you're at it?

---

Bindings/Java/tests/TestTables.java line 630 at r1 (raw file):

    }

    public static void test_TimeSeriesTableConcatenate() {

Very minor, but the test name suggests that the concatenate function is a static function of TimeSeriesTable. Consider renaming to test_TableUtilitiesConcatenate or something similar.

---

Bindings/Java/tests/TestTables.java line 638 at r1 (raw file):

        RowVector row = new RowVector(4, 1);
        System.out.println(table);
        // Append another row to the table.        

Trim trailing whitespace.

---

Bindings/Java/tests/TestTables.java line 641 at r1 (raw file):

        row.set(0, 2); row.set(1, 2); row.set(2, 2); row.set(3, 2);
        table.appendRow(0.2, row);
        // Make second table

nit:

Suggestion:

        // Create a second table.

---

Bindings/Java/tests/TestTables.java line 647 at r1 (raw file):

        row3.set(0, 3); row3.set(1, 3); row3.set(2, 3); row3.set(3, 3);
        table2.appendRow(0.3, row3);
        // combine

nit:

Suggestion:

        // Concatenate the tables.

---

OpenSim/Common/TableUtilities.h line 141 at r1 (raw file):

    /// Useful for stitching results segmented by AddBiomechanics
    /// Assumes secondTable time is greater than last time of firstTable
    /// Checks that headers are consistent otherwise throws

Some suggestions to improve the documentation, including removing references to AddBiomechanics:

Suggestion:

    /// Utility to concatenate two TimeSeriesTables into a new table. 
    /// Requires that both tables contain elements of the same type and 
    /// have the same number of columns with the same labels. The initial
    /// time point in the second table must be greater than the final 
    /// time in the first table.

---

OpenSim/Common/TableUtilities.h line 143 at r1 (raw file):

    /// Checks that headers are consistent otherwise throws
    template <typename T>
    static TimeSeriesTable_<T> concatenateTable(

I would suggest renaming this utility to concatenate(), which would be more in line with the naming conventions of the other utilities here (e.g., pad, resample, etc.).

---

OpenSim/Common/TableUtilities.h line 152 at r1 (raw file):

        // with resizeKeep so cost is not quadratic in regrowth To stitch tables
        // into a new one, create a clone before calling this function, then
        // repeatedly concatenate

It looks like there are some outdated/unrelated comments here. Some suggestions:

Suggestion:

        // Check that both tables have the same number of columns with 
        // equal column labels. Verify that the combined time vectors will
        // be monotonically increasing.

---

OpenSim/Common/TableUtilities.h line 176 at r1 (raw file):

        int nTotalRows = nRows1 + nRows2;

        // 1. merge time vectors

nit, formatting:

Suggestion:

        // Merge the time vectors.

---

OpenSim/Common/TableUtilities.h line 180 at r1 (raw file):

        mergedTimes.insert(mergedTimes.end(), secondTimes.begin(), secondTimes.end());

        // 2. Merge data

nit, formatting:

Suggestion:

        // Merge the data.

---

OpenSim/Common/Test/testDataTable.cpp line 1130 at r1 (raw file):

    CHECK(lastElement[2] == 9);

    // Check can't concatenate tableVec3 with itself since time will not be increasing

nit, formatting:

Suggestion:

    // Check that concatenating tableVec3 with itself throws since the time 
    // vector will not be monotonically increasing.

---

OpenSim/Common/Test/testDataTable.cpp line 1132 at r1 (raw file):

    // Check can't concatenate tableVec3 with itself since time will not be increasing
    SimTK_TEST_MUST_THROW(TableUtilities::concatenateTable<Vec3>(
            tableVec3Append, tableVec3););

Suggestion: you can use Catch2 test macros with Matchers to test that the correct exception message is generated. For example:

opensim-core/OpenSim/Moco/Test/testMocoTrack.cpp

Lines 33 to 36 in 4a1e891

	CHECK_THROWS_WITH(track.initialize(),
	Catch::Matchers::ContainsSubstring(
	"Property 'apply_tracked_states_to_guess' was "
	"enabled, but no states reference data was provided."));

---

OpenSim/Common/Test/testDataTable.cpp line 1135 at r1 (raw file):

    tableVec3Append.setColumnLabels({"col0", "col1", "col3"});
    // check throw if column labels mismatch

nit, formatting:

Suggestion:

    // Check that concatenating tables with inconsistent columns throws an
    // exception.
    tableVec3Append.setColumnLabels({"col0", "col1", "col3"});

---

OpenSim/Common/Test/testDataTable.cpp line 1137 at r1 (raw file):

    // check throw if column labels mismatch
    SimTK_TEST_MUST_THROW(TableUtilities::concatenateTable<Vec3>(
            tableVec3, tableVec3Append););

Same comment about using Matchers above.

[aymanhab]

agreed, thanks for the review @nickbianco The problem with the short name is that it leaves it unclear as to what it concatenates, it would definitely be clear enough if the class was called TimeSeriesTableUtilities but the class has other members. Nevertheless I went ahead with the short name, SWIG failed one of the instantiations (Rotation) otherwise all changes were made.

@aymanhab made 10 comments.
Reviewable status: 0 of 4 files reviewed, 15 unresolved discussions (waiting on nickbianco).

---

-- commits line 4 at r1:

Previously, nickbianco (Nick Bianco) wrote…

Like we've been doing recently, let's squash all commits into one before merging.

ok

---

Bindings/common.i line 393 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Would it make sense to add other template instantiations here, e.g., Rotation, Quaternion, while you're at it?

I added them, thank you, however SWIG couldn't handle Rotation though it handles Quaternions ok

---

Bindings/Java/tests/TestTables.java line 630 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Very minor, but the test name suggests that the concatenate function is a static function of TimeSeriesTable. Consider renaming to test_TableUtilitiesConcatenate or something similar.

In reality this works only for timeseriestable so I'm leaning to leave as is since TableUtilities context is not part of the function name.

---

Bindings/Java/tests/TestTables.java line 638 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Trim trailing whitespace.

Done.

---

OpenSim/Common/TableUtilities.h line 141 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Some suggestions to improve the documentation, including removing references to AddBiomechanics:

Done.

---

OpenSim/Common/TableUtilities.h line 143 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

I would suggest renaming this utility to concatenate(), which would be more in line with the naming conventions of the other utilities here (e.g., pad, resample, etc.).

Done.

---

OpenSim/Common/TableUtilities.h line 152 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

It looks like there are some outdated/unrelated comments here. Some suggestions:

Done.

---

OpenSim/Common/Test/testDataTable.cpp line 1132 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Suggestion: you can use Catch2 test macros with Matchers to test that the correct exception message is generated. For example:

opensim-core/OpenSim/Moco/Test/testMocoTrack.cpp

Lines 33 to 36 in 4a1e891

	CHECK_THROWS_WITH(track.initialize(),
	Catch::Matchers::ContainsSubstring(
	"Property 'apply_tracked_states_to_guess' was "
	"enabled, but no states reference data was provided."));

Done.

---

OpenSim/Common/Test/testDataTable.cpp line 1137 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Same comment about using Matchers above.

Done.

[nickbianco]

Thanks for the changes @aymanhab. On this pass I noticed that some of the error messages could be improved a bit, but otherwise is looking good.

@nickbianco reviewed 4 files and all commit messages, made 6 comments, and resolved 14 discussions.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on aymanhab).

---

Bindings/common.i line 393 at r1 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

I added them, thank you, however SWIG couldn't handle Rotation though it handles Quaternions ok

Sounds good. I've had issues with SimTK::Rotation as a template argument in the bindings too.

---

OpenSim/Common/TableUtilities.h line 159 at r2 (raw file):

        OPENSIM_THROW_IF(labels.size() != secondLabels.size(), Exception,
                "Cannot concatentate tables of inconsistent column labels..");

nit, formatting:

Suggestion:

                "Cannot concatentate tables with inconsistent column labels.");

---

OpenSim/Common/TableUtilities.h line 161 at r2 (raw file):

                "Cannot concatentate tables of inconsistent column labels..");
        OPENSIM_THROW_IF(!(labels == secondLabels), Exception,
                "Cannot concatentate tables of inconsistent column labels..");

nit, formatting:

Suggestion:

                "Cannot concatentate tables with inconsistent column labels.");

---

OpenSim/Common/TableUtilities.h line 164 at r2 (raw file):

        OPENSIM_THROW_IF(data.ncol() != secondData.ncol(), Exception,
                "Cannot concatentate tables of inconsistent number of "
                "columns..");

nit, formatting:

Suggestion:

                "Cannot concatentate tables with an inconsistent number of "
                "columns.");

---

OpenSim/Common/TableUtilities.h line 166 at r2 (raw file):

                "columns..");
        OPENSIM_THROW_IF(times[times.size() - 1] > secondTimes[0], Exception,
                "Cannot concatentate tables of not increasing times..");

I think this error message could be a bit clearer, in terms of guiding the user on how to fix issue. For example:

Suggestion:

                "Expected the final time point of the second point to be greater than "
                "the last time point of the first table, but concatenating the tables "
                "led to time vector that is not monotonically increasing.");

[aymanhab]

Done, thanks @nickbianco

@aymanhab made 1 comment.
Reviewable status: 2 of 4 files reviewed, 5 unresolved discussions (waiting on nickbianco).

[aymanhab]

All changes/suggestions were applied and tests pass. Thanks @nickbianco

@aymanhab made 3 comments and resolved 3 discussions.
Reviewable status: 2 of 5 files reviewed, 2 unresolved discussions (waiting on nickbianco).

---

Bindings/common.i line 393 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Sounds good. I've had issues with SimTK::Rotation as a template argument in the bindings too.

Done.

---

OpenSim/Common/TableUtilities.h line 166 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

I think this error message could be a bit clearer, in terms of guiding the user on how to fix issue. For example:

Done.

[nickbianco]

@nickbianco reviewed 3 files and all commit messages, and resolved 2 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on aymanhab).

[aymanhab]

Thanks @nickbianco 👍