Skip to content

Update TableUtilities::filterLowpass to apply padding after resampling#4020

Merged
nickbianco merged 4 commits intomainfrom
tab_op_trim_fix
Mar 10, 2025
Merged

Update TableUtilities::filterLowpass to apply padding after resampling#4020
nickbianco merged 4 commits intomainfrom
tab_op_trim_fix

Conversation

@nickbianco
Copy link
Member

@nickbianco nickbianco commented Feb 24, 2025

Fixes issue #4017

Brief summary of changes

This change updates TableUtilities::filterLowpass to apply padding after resampling, so that the initial time point in an uploaded table is preserved. This does not guarantee that the final time point will be preserved or the number of rows will be preserved as those may need to change based on the resampling frequency.

Drive-by: corrected a typo in the change log.

Testing I've completed

Added a unit test to testTableProcessor.cpp.

Looking for feedback on...

CHANGELOG.md (choose one)

  • updated.

This change is Reviewable

@nickbianco nickbianco requested a review from aymanhab March 10, 2025 03:30
Copy link
Member

@aymanhab aymanhab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nickbianco)


OpenSim/Common/TableUtilities.cpp line 151 at r1 (raw file):

    const int numRows = (int)table.getNumRows();
    OPENSIM_THROW_IF(numRows < 4, Exception,

I think if the numRows was < 4 we probably wouldn't get here? or it depends? maybe it wouldn't hurt anyway

Copy link
Member Author

@nickbianco nickbianco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @aymanhab!

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


OpenSim/Common/TableUtilities.cpp line 151 at r1 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

I think if the numRows was < 4 we probably wouldn't get here? or it depends? maybe it wouldn't hurt anyway

Neither isUniform or resampleWithInterval make this check, so I think it's worth including. Doesn't hurt to have it.

@nickbianco nickbianco merged commit cfd22c2 into main Mar 10, 2025
11 of 13 checks passed
@nickbianco nickbianco deleted the tab_op_trim_fix branch March 10, 2025 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants