-
Notifications
You must be signed in to change notification settings - Fork 23
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
Bug channel annot merge #275
Conversation
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.
Preventing merge between 2 annotations if at least one has a channe-specific annotation works for me. That's a good rule to start with and it can be changed later.
I am however greeted with this error when dragging a channel-specific annotation inside another one:
Traceback (most recent call last):
File "/home/scheltie/git/mscheltienne/mne-qt-browser/mne_qt_browser/_pg_figure.py", line 4645, in _region_changed
idx = self._get_onset_idx(region.old_onset)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/scheltie/git/mscheltienne/mne-qt-browser/mne_qt_browser/_pg_figure.py", line 4639, in _get_onset_idx
idx = np.where(self.mne.inst.annotations.onset == onset)[0][0]
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^
IndexError: index 0 is out of bounds for axis 0 with size 0
screncast-issue.webm
@mscheltienne I just checked and this is a bug even for general annotations. I can fix this in this or another PR. Untitled.mov |
Feel free to fix in this PR |
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.
@nmarkowitz can you verify that this works properly, and then add unit tests? Then we should be able to merge!
mne_qt_browser/_pg_figure.py
Outdated
logger.debug(f"New {self.description} region: {onset:.2f} - {offset:.2f}") | ||
# remove overlapping regions | ||
for region in overlapping_regions: | ||
# region.removeSingleChannelAnnots.emit(region) |
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.
Cruft?
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.
I tried to break it again, but failed! Looks fixed to me, thanks a lot!
@larsoner this one is ready to be merged |
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.
Looks good, thanks @nmarkowitz !
Reference issue
Example: Fixes #1234.
What does this implement/fix?
Fix issues when combining channel-based annotations with another annotation.
Additional information
Channel-based annotation can not merge with any other annotation.