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

Event detection for ic stride #60

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

AlzhraaIbrahim
Copy link
Collaborator

Adapt the event detection algorithm to work on "ic" stride that start with initial contact

Copy link
Member

@AKuederle AKuederle left a 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!

gaitmap/_event_detection_common/_event_detection_mixin.py Outdated Show resolved Hide resolved
gaitmap/_event_detection_common/_event_detection_mixin.py Outdated Show resolved Hide resolved
gaitmap/event_detection/_herzer_event_detection.py Outdated Show resolved Hide resolved
gaitmap/event_detection/_herzer_event_detection.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Mar 12, 2024

Codecov Report

Attention: Patch coverage is 30.69307% with 70 lines in your changes are missing coverage. Please review.

Project coverage is 90.27%. Comparing base (8510fe0) to head (d265ab7).

Files Patch % Lines
...tmap_mad/event_detection/_rampp_event_detection.py 18.82% 69 Missing ⚠️
gaitmap/event_detection/_herzer_event_detection.py 80.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@AlzhraaIbrahim
Copy link
Collaborator Author

@AKuederle Could you please review?

Copy link
Member

@AKuederle AKuederle left a 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.

gaitmap/_event_detection_common/_event_detection_mixin.py Outdated Show resolved Hide resolved
@@ -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
Copy link
Member

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

gaitmap/event_detection/_herzer_event_detection.py Outdated Show resolved Hide resolved
gaitmap/utils/stride_list_conversion.py Outdated Show resolved Hide resolved
gaitmap/utils/stride_list_conversion.py Outdated Show resolved Hide resolved
@@ -153,6 +157,7 @@ class RamppEventDetection(_EventDetectionMixin, BaseEventDetection):

ic_search_region_ms: Tuple[float, float]
Copy link
Member

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
Copy link
Member

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

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.

3 participants