Lowpass Filter Improved Uniform Sampling Check

[alexbeattie42]

Fixes issue #3968

Brief summary of changes

Implement an isUniform method to check if the vector is uniformly sampled. The implementation loosely follows the idea of the Matlab isUniform function. The implementation calculates the tolerance in this way:

isuniform verifies that the spacing between consecutive elements in numeric vector v does not deviate from the mean spacing by more than 4*eps(max(abs(v))), provided that the mean spacing is greater than that tolerance.

This may also fix this issue but I have not tested that myself yet.

Testing I've completed

Tested locally with this example and it fixes this issue. Will add unit tests soon.

Looking for feedback on...

@nickbianco and @aymanhab

• do you have feedback on this method for checking the uniform sampling
• where is the correct place for the uniform sampling check function to live. Maybe Signal?

CHANGELOG.md (choose one)

• Updated

---

This change is 

[nickbianco]

Nice work @alexbeattie42!

My main suggestion is to expose isUniform by adding it to CommonUtilities, with some documentation about how the uniformity checks work. You could then reference that in the updated documentation for TableUtilities::filterLowpass.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alexbeattie42)

---

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

template <typename T>
std::pair<bool,T> isUniform(const std::vector<T>& x) {

I think this would be a nice addition to CommonUtilities, as it might be useful for other table manipulation tools in OpenSim.

---

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

    // Resample if the sampling interval is not uniform.
    if (!uniformlySampled) {

Could you add some documentation in the header file describing the conditions under which a table will be resampled?

[alexbeattie42]

@nickbianco could you take another look?

Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @nickbianco)

---

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

Previously, nickbianco (Nick Bianco) wrote…

I think this would be a nice addition to CommonUtilities, as it might be useful for other table manipulation tools in OpenSim.

Done.

---

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

Previously, nickbianco (Nick Bianco) wrote…

Could you add some documentation in the header file describing the conditions under which a table will be resampled?

Done.

[nickbianco]

Thanks @alexbeattie42! Made a handful of minor request, but overall looks pretty good.

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @alexbeattie42)

---

OpenSim/Common/CommonUtilities.h line 149 at r2 (raw file):

 * uniform, this value will be the mean step size. If the input is a one element
 * vector, the step size will be NaN since a valid step size cannot be
 * calculated with only 1 element.

I think you could reduce the indentation level for both of these list items.

---

OpenSim/Common/CommonUtilities.h line 153 at r2 (raw file):

 * @note The function uses a tolerance based on the maximum absolute value of
 * the first and last elements in the vector, scaled by machine epsilon. If the
 * vector is empty or contains only one element, the behavior is undefined.

Use left-aligned indent for the text in this note (like the note below.

---

OpenSim/Common/CommonUtilities.h line 156 at r2 (raw file):

 * @note The function implementation draws inspiration from Matlab's `isuniform`.
 *       See https://mathworks.com/help/matlab/ref/isuniform.html for more details.
 * details

---

OpenSim/Common/CommonUtilities.cpp line 77 at r2 (raw file):

    const double step_size = (end - start) / static_cast<double>((length - 1));
    for (int i = 0; i < length; ++i) {
        v[i] = std::fma(i, step_size,start);

nit:

Suggestion:

        v[i] = std::fma(i, step_size, start);

---

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

     * rows is less than 4, or if the time intervals are not suitable for
     * resampling.
     *

nit:

---

OpenSim/Common/Test/testCommonUtilities.cpp line 135 at r2 (raw file):

        // Verify that the second value returned is the minimum step size found
        REQUIRE_THAT(result.second, Catch::Matchers::WithinAbs(0.03, tol));
    }

Not critical, but if you wanted to avoid "part 2" in the section headers here and below, you could scope the subtests like this (formatting isn't great here, but you get the idea):

SECTION("Non-uniform spacing") {
        {
            std::vector<double> vec3 = {1.0, 2.0, 3.5, 4.0};
            auto result = isUniform(vec3);
            REQUIRE(result.first == false);
            REQUIRE_THAT(result.second, Catch::Matchers::WithinAbs(0.5, tol));
      
            std::vector<double> vec4 = {1.0, 2.0, 3.0, 4.0, 5.1};
            result = isUniform(vec4);
            REQUIRE(result.first == false);
            REQUIRE_THAT(result.second, Catch::Matchers::WithinAbs(1.0, tol));
        }

        {
            std::vector<double> vec2 = {0.0, 0.5, 0.11, 0.16, 0.19, 0.24};
            auto result = isUniform(vec2);
      
            REQUIRE(result.first == false); // Should not be uniformly spaced
            // Verify that the second value returned is the minimum step size found
            REQUIRE_THAT(result.second, Catch::Matchers::WithinAbs(0.03, tol));
        }
    }

---

OpenSim/Common/Test/testCommonUtilities.cpp line 263 at r2 (raw file):

    REQUIRE_THAT(result_simtk.second, Catch::Matchers::WithinAbs(rate, tol));
    
}

nit: Add EOL line break.