-
Notifications
You must be signed in to change notification settings - Fork 3
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
Event detection for ic stride #60
base: master
Are you sure you want to change the base?
Event detection for ic stride #60
Conversation
…stride update _event_detection_mixin.py to check consistency of "ic" stride
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the "early" review :) Just some early comments! Let me know when I should have a detailed look.
But looks great so far!
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #60 +/- ##
==========================================
- Coverage 91.55% 90.27% -1.29%
==========================================
Files 85 85
Lines 4797 4884 +87
==========================================
+ Hits 4392 4409 +17
- Misses 405 475 +70 ☔ View full report in Codecov by Sentry. |
@AKuederle Could you please review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basic structure looks good.
I am not 100% sold on how we should handle the refinment of the IC values in general. Have a look at my comments and then we can maybe discuss via Zoom.
And we should probably add some tests for stride_type = ic.
@@ -53,7 +55,8 @@ class HerzerEventDetection(_EventDetectionMixin, BaseEventDetection): | |||
By default, all events ("ic", "tc", "min_vel") are detected. | |||
If `min_vel` is not detected, the `min_vel_event_list_` output will not be available. | |||
If "ic" is not detected, the `pre_ic` will also not be available in the output. | |||
|
|||
input_stride_type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a note here, that only segmented is supported by this algo
@@ -153,6 +157,7 @@ class RamppEventDetection(_EventDetectionMixin, BaseEventDetection): | |||
|
|||
ic_search_region_ms: Tuple[float, float] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens to this value, if the stride-type is ic? This seems to be ignored and you use other hardcoded thresholds.
@@ -38,6 +40,8 @@ class RamppEventDetection(_EventDetectionMixin, BaseEventDetection): | |||
By default, all events ("ic", "tc", "min_vel") are detected. | |||
If `min_vel` is not detected, the `min_vel_event_list_` output will not be available. | |||
If "ic" is not detected, the `pre_ic` will also not be available in the output. | |||
input_stride_type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to document what happens in case of IC strides as input. Probably something ot put in the Notes section
gaitmap_mad/gaitmap_mad/event_detection/_rampp_event_detection.py
Outdated
Show resolved
Hide resolved
gaitmap_mad/gaitmap_mad/event_detection/_rampp_event_detection.py
Outdated
Show resolved
Hide resolved
…irst part of stance] change detect_tc to return nan if no negative values were found
Adapt the event detection algorithm to work on "ic" stride that start with initial contact