Closed niksirbi closed 1 week ago
sonarcloud[bot]
commented
2 months ago 
Quality Gate passedIssues
0 New issues
0 Accepted issues
Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code
codecov[bot]
commented
2 months ago All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 99.77%. Comparing base (
ca4daf2) to head (13e2326). Report is 1 commits behind head on main.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
niksirbi
commented
2 weeks ago Thanks for the thorough review @lochhh!
xarray.sel to handle this. That said, I did choose to still catch one specific edge case (see relevant comment)ffill approach (instead of drop). It achieves the same result and is much more concise!main and dealt with merge conflicts.sonarcloud[bot]
commented
1 week ago Quality Gate passedIssues
0 New issues
0 Accepted issues
Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code
Description
What is this PR
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 existingmovementfunctionscompute_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
```python 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
startandstoparguments, as the user could easily select a time range usingds.position.sel(time=slice(0, 10)), before passing the data array tocompute_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:
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: