feat: Implement compute_directional_change for trajectory complexity#991
feat: Implement compute_directional_change for trajectory complexity#991vybhav72954 wants to merge 5 commits into
compute_directional_change for trajectory complexity#991Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #991 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 41 41
Lines 2829 2838 +9
=========================================
+ Hits 2829 2838 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
vybhav72954
force-pushed
the
feature/dc
branch
2 times, most recently
from
f175eff to
5ba7d84
Compare
niksirbi
left a comment
niksirbi
left a comment
There was a problem hiding this comment.
Thanks for the PR @vybhav72954!
Looks good overall but I found some subtle issues, detailed in inline comments.
The most important point is fixing the dt alignment.
Other than that, I also recommend exposing in_degrees, tidying the docstring, and adding a nonzero-DC test.
It should be good to merge after these changes.
| theta = compute_turning_angle(data, min_step_length=min_step_length) | ||
|
|
||
| time_coord = data["time"] | ||
| dt = time_coord.shift(time=-1) - time_coord.shift(time=1) |
There was a problem hiding this comment.
I think there is a temporal misalignment issue here!
Here's my thought process and let me know if you understand the matter differently.
compute_turning_angle defines θ at index i from positions i-2, i-1, i. So the turn physically spans the interval
But the PR computes i).
This doesn't matter for uniform time sampling, but will give wrong values for unevenly sampled data.
In the common fps case the time is uniformly sampled, which is why the tests pass.
Another consequence is that we unnecessarily lose data at the las step (as you have noted in the docstring).
I think we can remove that boundary constraint via the suggested fix.
Suggested fix: align dt to the turning interval:
dt = time_coord - time_coord.shift(time=2) # t_i − t_{i-2}This should be correct for non-uniform time and the last step also becomes valid.
The only NaNs are indices 0-1, to the corresponding docstring note will also need updating.
To 100% verify my theory and suggestion, we'd have to test this with non-uniformly sampled time dimension. Maybe a fixture like that could be added?
There was a problem hiding this comment.
Good point!
θ at index i is built from positions i-2, i-1, i, so dividing by t_{i+1} - t_{i-1} references a point (i+1) that isn't part of the angle.
Switched to dt = time_coord - time_coord.shift(time=2) which spans the turning angle's actual support. I've updated the math as you directed as well
Verified against a non-uniform-time fixture as well!
The old fixture actually fails it!
| pytest.param("stationary_paths", None, True, id="stationary"), | ||
| ], | ||
| ) | ||
| def test_directional_change_known_values( |
There was a problem hiding this comment.
The tests only assert DC = 0 (straight) and all-NaN (stationary). Neither exercises a known nonzero DC value, so neither the radians-per-time scaling nor the dt alignment is verified. A sharp_turn_paths fixture already exists is this file, btw. A case asserting a known nonzero DC, ideally with non-uniform time coords, would have caught the alignment issue.
There was a problem hiding this comment.
I agree, for better coverage, added test_directional_change_nonzero_value
Also parametrized over in_degrees, so the radians scaling is pinned down too
| nan_warn_threshold : float, optional | ||
| If any point track in the data has at least (:math:`\ge`) | ||
| this proportion of values missing, a warning will be emitted. | ||
| Defaults to 0.2 (20%). |
There was a problem hiding this comment.
Now that I thought about it a bit further, perhaps we shouldn't expose nan_warn_threshold here. It makes sense to expose it in metrics that summarise across time (like compute_path_length), because those will be severely skewed by a high NaN proportion.
Since this function outputs per frame, the natural expectation is that NaNs will just propagate from input to output. I think the warning here may end up being too annoying for users. If half of my time points are missing, I may still want to know the DC values of the valid time-points, without being pestered by warnings.
What do you think?
|
@niksirbi Updated! Main fix is the I have made small changes to the docstrings and references, as you directed. I have left the main talking points unresolved! I belive i have addressed all fo your comments, please let me know if i am missing anything! |
for more information, see https://pre-commit.ci
|
2 participants
Description
What is this PR
Why is this PR needed?
Implements
compute_directional_change(Fixes #907), one of four trajectory complexity metrics inmovement's current scope. DC quantifies how rapidly an animal changes its heading per unit time; useful for distinguishing slow tortuous paths from fast straight ones, which purely geometric metrics like sinuosity can't tell apart.What does this PR do?
compute_directional_changeinmovement/kinematics/path.py.The directional change at step$i$ is the absolute turning angle divided by the time interval:
References
How has this PR been tested?
Two new parametrized test functions in
tests/test_unit/test_kinematics/test_path.py:test_directional_change_known_values— DC = 0 on straight-line motion (interior steps), DC = NaN on stationary paths; boundary time steps (t=0, t=1, t=-1) are always NaN.test_directional_change_across_time_rangesIs this a breaking change?
No - purely additive. New public function, no existing API touched.
Does this PR require an update to the documentation?
The new function has a complete numpydoc docstring with math, reference, Notes section,
See Also, andExamples.Checklist: