-
Notifications
You must be signed in to change notification settings - Fork 22
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
Vizualization framework for channel specific annotations #202
Conversation
Co-Authored-by: Scott Huberty <[email protected]>
should handle a key error that will be thrown when trying to access "ch_names" in an Annotation without that key. This case occurs in the mne base tests.
This almost works: Of course, the sample audvis dataset always seems to present the most complex scenario (floating point sfreq, etc) 🙂 If I run this:
I get this plot "MEG 0143" is in @drammock I think this has to do with our "dirty hack", and that these MEG channels are not ordered in the plot by their number (i.e. "MEG 0133", "MEG 0143", "MEG 0142"). |
ah, right you are. Our dirty hack will not work. Maybe check if the channel order is separately specified / kept track of somewhere in |
Aesthetically pleasing despite the indexing bug! For the indexing I would look at how we figure out when you click on a channel name Y label which channel is actually bad. Indexing correspondence between channel names and what is currently shown should be used there I think |
I could not figure out how to find the y coordinates of the FillBetweenItems. So this only tests that the FillBetweenItems, i.e. the shaded rectangles around each channel in annotation["ch_names"], match the number of channel names in the annot["ch_names"] key
@scott-huberty did you test interactivity? Using this code:
pressing "a" on the keyboard to enter annot mode and shrinking the annot I see the ch-specific one does not shrink: Can you replicate? |
I did not test that, and can replicate 😅 Looking into it |
Code looks good other than the interactivity issue! |
FYI @scott-huberty I changed your wording in the top-comment from "supersedes" to "closes" because the latter does GitHub closing magic |
Going to stop for the day soon, but FYI the fix that we thought we had for this (updating the |
xData was just a list, right? There is probably a .update or something that you need to call. But simpler would probably be too call https://pyqtgraph.readthedocs.io/en/latest/api_reference/graphicsItems/plotcurveitem.html#pyqtgraph.PlotCurveItem.setData on the two curves that define the fill between. If that doesn't fix it, calling that then https://pyqtgraph.readthedocs.io/en/latest/api_reference/graphicsItems/fillbetweenitem.html#pyqtgraph.FillBetweenItem.setCurves should do it. It's likely that these |
Exactly! The |
Let's make sure this works.. Lot of conflicts due to black formatting PR
…fic annot - This fix only works if you change the annot region start/stop using the spinboxes - TODO: Make FillBetweenItems stay synced with annot region if you drag the annot region in the plotting area
4866137
to
c1909b2
Compare
the I had to rebase with main after #203, and force push my changes to this remote branch. Before doing that I made a local copy of the state of this remote branch, in case I realize that i've done something terribly wrong.. |
At the end of the day almost all that matters for our squash-and-merge is the |
This I think is where the |
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'll commit these and merge, thanks @scott-huberty !
from mne import Annotations | ||
from pyqtgraph.graphicsItems.FillBetweenItem import FillBetweenItem | ||
|
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.
Shouldn't nest in tests
Closes #200
Closes #196
(EDIT) starts to close mne-tools/mne-python#8946 . I think we will need to implement the same funcitionality for the matplotlib backend before closing that issue.