-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add tolerance parameter to events_from_annotations to #12321
Comments
Can you look at PR #12311 to see if it makes this no longer a problem? |
I like #12311 update but the problem is different from this PR. #12311 is useful if the annotations are precisely segmented into epoch length, But this can not be used if the annotations have duration of epoch length x N (N>=1)
In this case I want epochs of 30sec(W), 30sec(1), 30sec(2), 30sec(2), 30sec(3). This is possible with the traditional method:
So, But if I make Epochs from raw directly with #12311 code,
so, the 4th epoch is skipped. |
I'm ready to make PR if it seems useful. |
I think it's a useful PR. I would even consider tol to be by default eg some eps * sfreq. I guess it would not change the current behavior and would fix your issue. thx @rcmdnk for the meticulous investigation ! |
@agramfort |
Describe the new feature or enhancement
Add
tol
parameter toannotations_from_events
to avoid omitting epochs by rounding errors.Describe your proposed implementation
In the function named events_from_annotations, there is a section of code as follows:
However, if we consider values like
In theory, this region should be obtained as a 30-second epoch, but due to rounding errors, it results in:
This causes good_events to contain False, and thus, this epoch is not captured.
However, in reality, this interval's data should be fully treated as an epoch. Therefore, I'm considering introducing a tolerance and modifying the code like this:
By adding a small tol, we can avoid the situation mentioned above. If tol is sufficiently smaller than the frequency, it should not erroneously include intervals that are not supposed to be selected.
This is safe as long as
use_rounding = True
.If
use_rounding = False
, the first data of the epoch could be overlapped with the previous epoch. (iftol = 0
, such an epoch is not created.)But even this case, I think it is better to keep the epoch.
By setting the default value of tol to 0, we can maintain the same results as currently obtained, as long as it is used in the same way as now.
Describe possible alternatives
Currently, I am using Annotations that are precisely segmented into 30-second intervals. By setting chunk_duration = 0, I can create indexes without needing to calculate the duration.
This approach ensures that all epochs are included in the output data.
However, sometimes the annotations have durations that are multiple times the length of an epoch. In these cases, we need to modify the annotations before applying them to the Raw data.
Additional context
No response
The text was updated successfully, but these errors were encountered: