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

Fix annotation of last sample #308

Merged
merged 4 commits into from
Jan 27, 2025
Merged

Conversation

richardkoehler
Copy link
Contributor

Reference issue

Fixes issue #12893 in mne-python (mne-tools/mne-python#12893)
The issue is also discussed in PR #12895 in mne-python (mne-tools/mne-python#12895)

What does this implement/fix?

Currently, when annotating bad epochs in mne-qt-browser, annotations were limited to the last sample of the recording, so that the last sample of the recording would not actually be rejected.
This fixes this issue, and additionally allows for 1-sample annotations at the start (t=0) and end (t=xmax) of the recording.

  • raise upper limit of RawViewBox to xmax + 1 sample, so that new annotations can extend above the recording
  • raise upper limit of AnnotRegion to xmax + 1 sample for all annotations
  • to allow for 1-sample annotations at the beginning (0) and end (xmax) of recording in the AnnotationDock :
    • raise maximum of an annotation start to xmax
    • lower the minimum of an annotation end/stop to 0
    • raise maximum of an annotation end to xmax + 1 sample

- raise upper limit of RawViewBox to xmax + 1 sample, so that new annotations can extend above the recording
- raise upper limit of AnnotRegion to xmax + 1 sample for all annotations
- to allow for 1-sample annotations at the beginning (0) and end (xmax) of recording in the AnnotationDock :
    - raise maximum of an annotation start to xmax
    - lower the minimum of an annotation end/stop to 0
    - raise maximum of an annotation end to xmax + 1 sample
@larsoner
Copy link
Member

@richardkoehler can you look at modifying some test such that it would fail on main but pass on this PR? We have some tests that (via code) click-and-drag annotations I think, so I think you should be able to follow those and adapt appropriately

Add tests that check:
- annotations that extend above the recording end
- 1-sample (0 duration) annotations
@richardkoehler
Copy link
Contributor Author

@larsoner Sure, it took me a while to get the hang of the simulated clicks and drags, but I managed now. I implemented two new tests that pass here but fail in main. One other test also fails in main, but passes in mine, since it was checking start<stop (and vice versa), which we changed to <=.
I downsampled the testing data in my tests, which is not strictly necessary, but speeds them up.

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful, thanks @richardkoehler !

@larsoner larsoner merged commit 845d7a7 into mne-tools:main Jan 27, 2025
20 checks passed
@richardkoehler
Copy link
Contributor Author

No problem, thanks for the support and the merge!

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.

2 participants