feat: implement compute_path_sinuosity core logic (draft)#981
Conversation
isha822
force-pushed
the
feature/sinuosity
branch
from
dc19858 to
47b5c66
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #981 +/- ##
===========================================
- Coverage 100.00% 99.36% -0.64%
===========================================
Files 41 41
Lines 2838 2857 +19
===========================================
+ Hits 2838 2839 +1
- Misses 0 18 +18 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I've updated the PR to address all your review comments. I also pulled in the latest changes from main and resolved the merge conflicts, so this is now fully synced with the recent start/stop removals and the new compute_directional_change function. Could you take a quick look at the updated core logic? If the math and structure look good to you now, I'll go ahead and push the updated test suite! |
niksirbi
left a comment
niksirbi
left a comment
There was a problem hiding this comment.
Thanks for keeping an eye on this PR and on updating it based on the latest change in main @isha822 !
I had a look at the current implementation and left some comments.
I think it's time to move onto writing tests for this function, in test_path.py. There could be subtle implementation issues we've both missed, but tests are the perfect means to reveal them. I would pay particular attention to checking that the edge cases are resolved as the docstring claims.
Feel free to take your time with the tests. More important to get this right than to get it done fast.
|
|
||
| result.name = "sinuosity" | ||
| result.attrs["long_name"] = "Path Sinuosity" | ||
| result.attrs["units"] = "1/sqrt(length)" |
There was a problem hiding this comment.
We don't yet use the units attribute for any other metric. I would leave it out for consistency.
Co-authored-by: Niko Sirmpilatze <niko.sirbiladze@gmail.com>
Co-authored-by: Niko Sirmpilatze <niko.sirbiladze@gmail.com>
Co-authored-by: Niko Sirmpilatze <niko.sirbiladze@gmail.com>
Co-authored-by: Niko Sirmpilatze <niko.sirbiladze@gmail.com>
Co-authored-by: Niko Sirmpilatze <niko.sirbiladze@gmail.com>
Co-authored-by: Niko Sirmpilatze <niko.sirbiladze@gmail.com>
|
Thank you @niksirbi for the review. The suggestions are very helpful. I'll work on incorporating them, cover the relevant edge cases, and add checks to catch potential anomalies. I'll update the PR as soon as I can. |
|
|
Hey @niksirbi Just resolved the merge conflicts and synced all your suggestions from earlier! You'll notice the CI is failing right now. Aside from a few linting/formatting quirks I need to smooth out from the merge, the main test suite is failing on one specific biological edge case I added: test_path_sinuosity_perfect_reversals. I was thinking about real-world scenarios and realized there's a math quirk. If we track an animal perfectly pacing back and forth with a constant stride (A -> B -> A -> B), the mean turning angle cosine becomes exactly -1 and the step variation is 0. When that hits Eq 8, it throws a divide-by-zero warning and spits out inf. What do you think? is this test valid?, let me know if im missing something or got anything wrong. |
2 participants
Opening this as a draft to get early eyes on the core logic and math implementation (Benhamou 2004).
Key implementation details:
I will start adding the pytest suite for this once the core mathematical approach looks good.