Implementing rolling median function for metrics.py #38

frey-perez · 2024-05-15T14:04:19Z

resolves Task: Implement rolling median function in metrics.py #30

This function implements the rolling median function, a function meant to pre-process acceleration data before determining angle_relative_to_horizontal values. All relevant unit tests are included.

codecov · 2024-05-15T14:06:00Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (fd1a714) to head (9d91309).

❗ Current head 9d91309 differs from pull request most recent head ae24b23

Please upload reports for the commit ae24b23 to get more accurate results.

Additional details and impacted files

@@              Coverage Diff              @@
##           development       #38   +/-   ##
=============================================
  Coverage       100.00%   100.00%           
=============================================
  Files                4         4           
  Lines              113       120    +7     
=============================================
+ Hits               113       120    +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nx10 · 2024-05-15T14:09:15Z

Why not use https://docs.pola.rs/py-polars/html/reference/expressions/api/polars.Expr.rolling_median.html and other polars.rolling_* methods instead of implementing all of these?

ReinderVosDeWael · 2024-05-15T14:53:11Z

src/wristpy/processing/metrics.py

+
+
+def rolling_median(
+    acceleration: models.Measurement, window_size: int = 51


Why is this in measurement units, whereas rolling_median and rolling_std are in seconds?

ReinderVosDeWael · 2024-05-15T14:53:26Z

src/wristpy/processing/metrics.py

@@ -53,3 +56,59 @@ def angle_relative_to_horizontal(
    angle_degrees = np.degrees(angle_radians)

    return models.Measurement(measurements=angle_degrees, time=acceleration.time)
+
+
+def rolling_median(


Shouldn't this be in core/computations.py see rolling_std()

src/wristpy/processing/metrics.py

ReinderVosDeWael · 2024-05-15T14:59:43Z

src/wristpy/processing/metrics.py

+    if window_size <= 1:
+        raise ValueError("window size must be greater than 1.")
+
+    if window_size % 2 == 0:


Two options:

Raise a ValueError.

Don't do this; even numbers are fine.

If 1 add test, if 2 is covered by regular tests.

ReinderVosDeWael · 2024-05-15T15:00:25Z

src/wristpy/processing/metrics.py

+        )
+
+    radius = window_size // 2
+    raw_data = acceleration.measurements


Don't alias it.

ReinderVosDeWael · 2024-05-15T15:09:53Z

tests/unit/test_metrics.py

@@ -79,3 +79,57 @@ def test_angle_relative_to_horizontal(
    assert angle_z_results.measurements.shape == (1,), (
        f"Expected anglez shape: {(1,)}," f"got({angle_z_results.measurements.shape})"
    )
+
+
+def test_rolling_median_wrong_ndims() -> None:


See comment above; this should work.

ReinderVosDeWael · 2024-05-15T15:11:39Z

tests/unit/test_metrics.py

+        metrics.rolling_median(test_measurement)
+
+
+def test_rolling_median_window_size() -> None:


Let numpy handle this; we dont need to handle it.

ReinderVosDeWael · 2024-05-15T15:19:08Z

src/wristpy/processing/metrics.py

@@ -53,3 +56,59 @@ def angle_relative_to_horizontal(
    angle_degrees = np.degrees(angle_radians)

    return models.Measurement(measurements=angle_degrees, time=acceleration.time)
+
+
+def rolling_median(


Rename to moving for consistency with moving_mean and moving_std

ReinderVosDeWael · 2024-05-15T15:20:40Z

Why not use https://docs.pola.rs/py-polars/html/reference/expressions/api/polars.Expr.rolling_median.html and other polars.rolling_* methods instead of implementing all of these?

Implementing moving_mean function in metrics.py #31

Adding moving_std to computations #34

Implementing rolling median function for metrics.py #38

Fair point - for now, as there's some design questions to be answered regarding handling of even window size and unit of window size, lets leave this for now. I'll make an issue out of it though so we can reassess this when those issues are resolved.

-Renamed rollingmedian to moving Median -Moved moving median function into computations -Moved unit tests for moving_median to test_computations.py -changed moving median to Polars implementation. -window size is based on time, matching the other moving functions -

ReinderVosDeWael

Two relatively small comments, but see if you can make sure the moving_median matches the input implementation of moving_mean so we can use them interchangeably.

ReinderVosDeWael · 2024-05-21T13:13:23Z

src/wristpy/core/computations.py

+        accelerometer data in an np.array and 2) time, a pl.Series containing
+        datetime.datetime objects.
+
+        window_size: The size of the overlapping window within which the median will be


overlapping with what? Isn't it just the size of the window?

By overlapping i just mean the windows will overlap in terms of which samples make up the window, as the window is sliding down one sample at a time. Should I edit it, or just remove it if that's implicitly understood?

Ah right, how about

"""Applies moving median to acceleration data. Step size for the window is hard-coded to 1 sample. Args: ... """

yes exactly, I'll make that edit.

ReinderVosDeWael · 2024-05-21T13:19:15Z

src/wristpy/core/computations.py

+    measurements_polars_df = measurements_polars_df.set_sorted("time")
+    offset = -((window_size // 2) + 1)
+    offset_str = str(offset) + "s"
+    moving_median_df = measurements_polars_df.select(


This function will not be able to take the moving median of anything but acceleration as it will fail on data that isn't N-by-3. Are we sure that we never need this for any other measurement? If no, can we just copy the implementation of moving_mean as I believe this one is valid for any N-by-M array.

That's true I can edit this to cover any amount of cols like so:

moving_median_df = measurements_polars_df.select( [ pl.median("*").rolling( index_column="time", period=f"{window_size}s", offset=offset_str ) ] ) return models.Measurement( measurements=moving_median_df.drop("time").to_numpy(), time=measurements_polars_df["time"], )

-changed the wording of doc string for clarity.

frey-perez added 6 commits May 13, 2024 16:52

Initial rolling median function

75fc702

Adding tests and docstrings to rolling_median

4242601

re-adding old tests, accidentally deleted

22077df

Minor type hinting error

2acc12d

Merge branch 'development' into freymon_dev_rolling_median

96fbce5

Small doc string change

50bb392

frey-perez requested a review from ReinderVosDeWael May 15, 2024 14:04

ReinderVosDeWael requested changes May 15, 2024

View reviewed changes

ReinderVosDeWael reviewed May 15, 2024

View reviewed changes

ReinderVosDeWael mentioned this pull request May 15, 2024

Task: Consider switching moving_* to polars functions. #39

Closed

frey-perez added 2 commits May 20, 2024 11:35

Small docstring change

918e4f7

frey-perez requested a review from ReinderVosDeWael May 20, 2024 15:44

ReinderVosDeWael requested changes May 21, 2024

View reviewed changes

frey-perez added 2 commits May 21, 2024 12:27

Moving median can handle any number of cols

9d91309

-changed the wording of doc string for clarity.

Doc string ruff error

ae24b23

ReinderVosDeWael approved these changes May 21, 2024

View reviewed changes

frey-perez merged commit d91eace into development May 21, 2024
6 checks passed

frey-perez deleted the freymon_dev_rolling_median branch May 21, 2024 22:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementing rolling median function for metrics.py #38

Implementing rolling median function for metrics.py #38

frey-perez commented May 15, 2024

codecov bot commented May 15, 2024 •

edited

Loading

nx10 commented May 15, 2024

ReinderVosDeWael May 15, 2024

ReinderVosDeWael May 15, 2024

ReinderVosDeWael May 15, 2024

ReinderVosDeWael May 15, 2024

ReinderVosDeWael May 15, 2024

ReinderVosDeWael May 15, 2024

ReinderVosDeWael May 15, 2024

ReinderVosDeWael May 15, 2024

ReinderVosDeWael commented May 15, 2024

ReinderVosDeWael left a comment

ReinderVosDeWael May 21, 2024

frey-perez May 21, 2024

ReinderVosDeWael May 21, 2024

frey-perez May 21, 2024

ReinderVosDeWael May 21, 2024

frey-perez May 21, 2024



		def rolling_median(
		acceleration: models.Measurement, window_size: int = 51

		metrics.rolling_median(test_measurement)


		def test_rolling_median_window_size() -> None:

Implementing rolling median function for metrics.py #38

Implementing rolling median function for metrics.py #38

Conversation

frey-perez commented May 15, 2024

codecov bot commented May 15, 2024 • edited Loading

Codecov Report

nx10 commented May 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ReinderVosDeWael commented May 15, 2024

ReinderVosDeWael left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 15, 2024 •

edited

Loading