Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement compute_speed and compute_path_length #280

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

niksirbi
Copy link
Member

@niksirbi niksirbi commented Aug 23, 2024

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

What does this PR do?

It introduces two new functions in the kinematics module:

  • compute_speed: quite straightforward to implement, it just returns the norm of velocity, using existing movement functions
  • compute_path_length: in theory also straightforward, as it should just be the sum of the norms of displacement vectors, but I spent a lot of time debating how to handle missing values. See extensive discussion on zulip.

In the end I opted for providing two alternative nan policies, and emitting a warning if too many values are missing. Expand the dropdown below for more details.

compute_path_length() function signatures and docstring
def compute_path_length(
    data: xr.DataArray,
    start: float | None = None,
    stop: float | None = None,
    nan_policy: Literal["drop", "scale"] = "drop",
    nan_warn_threshold: float = 0.2,
) -> xr.DataArray:
    """Compute the length of a path travelled between two time points.

    The path length is defined as the sum of the norms (magnitudes) of the
    displacement vectors between two time points ``start`` and ``stop``,
    which should be provided in the time units of the data array.
    If not specified, the minimum and maximum time coordinates of the data
    array are used as start and stop times, respectively.

    Parameters
    ----------
    data : xarray.DataArray
        The input data containing position information in Cartesian
        coordinates, with ``time`` and ``space`` among the dimensions.
    start : float, optional
        The time to consider as the path's starting point. If None (default),
        the minimum time coordinate in the data is used.
    stop : float, optional
        The time to consider as the path's end point. If None (default),
        the maximum time coordinate in the data is used.
    nan_policy : Literal["drop", "scale"], optional
        Policy to handle NaN (missing) values. Can be one of the ``"drop"``
        or ``"scale"``. Defaults to ``"drop"``. See Notes for more details
        on the two policies.
    nan_warn_threshold : float, optional
        If more than this proportion of values are missing in any point track,
        a warning will be emitted. Defaults to 0.2 (20%).

    Returns
    -------
    xarray.DataArray
        An xarray DataArray containing the computed path length.
        Will have the same dimensions as the input data, except for ``time``
        and ``space`` which will be removed.

    Notes
    -----
    Choosing ``nan_policy="drop"`` will drop NaN values from each point track
    before computing path length. This equates to assuming that a track
    follows a straight line between two valid points flanking a missing
    segment. Missing segments at the beginning or end of the specified
    time range are not counted. This approach tends to underestimate
    the path length, and the error increases with the number of missing
    values.

    Choosing ``nan_policy="scale"`` will adjust the path length based on the
    the proportion of valid segments per point track. For example, if only
    80% of segments are present, the path length will be computed based on
    these and the result will be divided by 0.8. This approach assumes
    that motion dynamics are similar across present and missing time
    segments, which may not be the case.

    """

I'm in two minds as to whether we need the start and stop arguments, as the user could easily select a time range using ds.position.sel(time=slice(0, 10)), before passing the data array to compute_path_length(). I've chosen to include them for now, but I'm open to counter-arguments, CC @b-peri.

References

Closes #147

How has this PR been tested?

For speed, I used the existing kinematics tests (added "speed" as a parameter) with a bit of modification needed.

Testing for the path length was harder because:

  • this variable lacks both time and space dimensions, so modifying existing kinematics tests to account for it would b complicated
  • With the two aforementioned nan_policies, there are several tricky edge cases, and I tried to cover them all. The uniform linear motion fixture has been instrumental in helping me implement , debug and test these nan policies.
  • I also had to test that the nan warning is emitted in the right scenarios and contains specific messages.

As a result I ended up adding 3 new unit tests to account for all of the above, but suggestion to streamline the tests are welcome.

Is this a breaking change?

No.

Does this PR require an update to the documentation?

API docs should be automatically updated. We might consider mentioning the new functions in some of the examples in the future, but not necessary right now.

Checklist:

  • The code has been tested locally
  • Tests have been added to cover all new functionality
  • The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

Copy link

sonarcloud bot commented Aug 23, 2024

Copy link

codecov bot commented Aug 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.79%. Comparing base (b399ce0) to head (d089a9c).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #280      +/-   ##
==========================================
+ Coverage   99.77%   99.79%   +0.01%     
==========================================
  Files          15       15              
  Lines         909      957      +48     
==========================================
+ Hits          907      955      +48     
  Misses          2        2              

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

@niksirbi niksirbi changed the title WIP: implement compute_speed and compute_distance_traveled Implement compute_speed and compute_path_length Oct 8, 2024
@niksirbi niksirbi marked this pull request as ready for review October 10, 2024 16:35
@niksirbi niksirbi requested a review from lochhh October 10, 2024 16:35
Copy link

sonarcloud bot commented Oct 22, 2024

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.

Consider adding a distance and speed property to accessor
1 participant