Skip to content

Uniform Time Sampling in XsenseDataReader and ADPMDataReader#3977

Merged
nickbianco merged 1 commit intoopensim-org:mainfrom
gateway240:uniform-time
Dec 18, 2024
Merged

Uniform Time Sampling in XsenseDataReader and ADPMDataReader#3977
nickbianco merged 1 commit intoopensim-org:mainfrom
gateway240:uniform-time

Conversation

@alexbeattie42
Copy link
Contributor

@alexbeattie42 alexbeattie42 commented Nov 27, 2024

Fixes issue #3976

Brief summary of changes

Implementation of the method described in this issue

Testing I've completed

Tested locally. Will add tests

Looking for feedback on...

@aymanhab and @nickbianco

  • do you have feedback on this method for creating the uniformly spaced interval
  • where is the correct place for the uniform time creation function to live. Maybe TableUtilities?

CHANGELOG.md (choose one)

  • updated

This change is Reviewable

@alexbeattie42 alexbeattie42 changed the title Uniform time Uniform Time Sampling in XsenseDataReader Nov 27, 2024
@nickbianco
Copy link
Member

@alexbeattie42, would the existing CommonUtilities::createVectorLinspace be sufficient here?

@alexbeattie42
Copy link
Contributor Author

alexbeattie42 commented Nov 27, 2024

@nickbianco Thanks for the suggestion! I updated the the code to use the createVectorLinspace function and it is working properly. Is there any simple way to avoid the copy from SimTK::Vector to std::vector<double>.

    std::vector<double> times(times_vec.getContiguousScalarData(),times_vec.getContiguousScalarData() + times_vec.size());

I looked at modifying the createTablesFromMatrices method here but there does not appear to be a constructor for the TimeSeriesTable that accepts a SimTK::Vector. This kind of thing is done here but I'm curious if there's a better way that you know of.

@alexbeattie42 alexbeattie42 changed the title Uniform Time Sampling in XsenseDataReader Uniform Time Sampling in XsenseDataReader and ADPMDataReader Nov 27, 2024
@nickbianco
Copy link
Member

I suppose we could add another linspace function to CommonUtilities that returns a std::vector rather than a SimTK::Vector.

@alexbeattie42 alexbeattie42 marked this pull request as ready for review December 5, 2024 07:32
@alexbeattie42
Copy link
Contributor Author

@nickbianco could you take another look?

Copy link
Member

@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.

Nice work @alexbeattie42!

Just a few nitpicks to address.

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @alexbeattie42)


OpenSim/Common/XsensDataReader.cpp line 165 at r3 (raw file):

        }
    }
    const double timeIncrement = 1 / dataRate;

nit: use 1.0 to avoid implicit conversion:

Suggestion:

    const double timeIncrement = 1.0 / dataRate;

OpenSim/Common/APDMDataReader.cpp line 192 at r3 (raw file):

    }
    // Trim Matrices in use to actual data and move into tables
    const double timeIncrement = 1 / dataRate;

nit: use 1.0 to avoid implicit conversion:

Suggestion:

    const double timeIncrement = 1.0 / dataRate;

OpenSim/Common/Test/testCommonUtilities.cpp line 2 at r3 (raw file):

/* --------------------------------------------------------------------------*
*                         OpenSim:  testCommonUtilities.cpp                  *

nit: misaligned comment box (this line and the ones below):

Suggestion:

/* --------------------------------------------------------------------------*
 *                         OpenSim:  testCommonUtilities.cpp                 *

Copy link
Contributor Author

@alexbeattie42 alexbeattie42 left a comment

Choose a reason for hiding this comment

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

@nickbianco Thanks! These are fixed now

Reviewable status: 1 of 4 files reviewed, all discussions resolved (waiting on @nickbianco)

…r` type, add unit tests, and fix the related bug (opensim-org#3976) in IMU DataReaders
Copy link
Member

@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.

Reviewed 5 of 5 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @alexbeattie42)

@nickbianco nickbianco merged commit 097410f into opensim-org:main Dec 18, 2024
@nickbianco
Copy link
Member

Thanks @alexbeattie42!

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