-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
FIX: Set zorder for y-axis spine in plot_evoked (#13492) #13506
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
base: main
Are you sure you want to change the base?
FIX: Set zorder for y-axis spine in plot_evoked (#13492) #13506
Conversation
|
Hey @mohit-malpote I dont think this fixes the issue? :
I ran this tutorial: https://mne.tools/stable/auto_tutorials/evoked/30_eeg_erp.html#sphx-glr-auto-tutorials-evoked-30-eeg-erp-py By running this command from the mne-python repository root directory (after checking out this PR's branch): ipython -i tutorials/evoked/30_eeg_erp.py |
mne/viz/evoked.py
Outdated
| # Put back the y limits as fill_betweenx messes them up | ||
| ax.set_ylim(this_ylim) | ||
|
|
||
| ax.spines["left"].set_zorder(10) |
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 think something like this should work.
ax.spines[:].set_zorder(max(l.get_zorder() for l in ax.get_lines()) + 1)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.
ok @mistraube i will try to do that
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.
1934fbf to
ee31dd3
Compare
Is your screenshot from the try with fixed zorder 50? Would make sence to me since only some (of the 59) channels are plotted above the axis. For me this works: ax.spines[:].set_zorder(max(l.get_zorder() for l in ax.get_lines()) + 1)
|
…ne-tools#13492) -- Final robust dynamic fix
e60e1fc to
d560363
Compare
|
I have verified the fix locally against the tutorial, and it is now working correctly! The vertical Y-axis spine is clearly visible on top of all the data traces. I have pushed the final commit implementing the robust z-order fix. Thanks again to @mistraube for providing the key insight for the dynamic zorder calculation!
|
scott-huberty
left a comment
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.
Hey @mohit-malpote - If you create an account on https://circleci.com (you can sign in with your GitHub account), then those failing CI's might pass. Right now they are failing because as a precaution, we don't let CircleCI CI's run if the user opening the PR does not have a CircleCI account.
We also need you to add a change log entry. Check out our contributing guide to learn how to do this!
| ax.spines[:].set_zorder(max(l.get_zorder() for l in ax.get_lines()) + 1) | ||
|
|
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.
| ax.spines[:].set_zorder(max(l.get_zorder() for l in ax.get_lines()) + 1) | |
| # Ensure the axis spines are drawn above all Line2D artists | |
| max_zorder = max((line.get_zorder() for line in ax.get_lines()), default=0) + 1 | |
| ax.spines[:].set_zorder(max_zorder) |
- Locally, Ruff complains about
lbeing an ambiguous variable name (and I agree). - I split @mistraube 's suggestion into 2 lines to make the code a little more verbose, and added a code comment as hint to other devs.
- the
max(..., default=0)is just a little defensive programming to guard against a case where there are noLine2Dartists to iterate through, which could throw an error. We may never actually ever hit that case in the wild, but better safe than sorry.
@mohit-malpote is it clear for you why @mistraube 's approach is preferable to hard coding a z-order like 10 or even 50? The upshot is that each of these Line2D artists correspond to one channel on the plot. A single, hardcoded z-order won't work for all use-cases (Some EEG nets have 256 or more channels!).. So computing the maximum z-order dynamically is the way to go.




Here is a perfect description for your Pull Request (PR) based on the fix you implemented, filling out the expected MNE-Python layout:
Reference issue (if any)
Fixes #13492.
What does this implement/fix?
This PR fixes a plotting regression where the Y-axis spine (the vertical line) was drawn behind the plotted data lines, causing it to be obscured, especially by the Global Field Power (GFP) or channel traces in butterfly plots.
The fix involves explicitly setting a high Matplotlib zorder for the left axis spine in mne.viz._plot_lines:
A single line was added: ax.spines['left'].set_zorder(10)
This ensures the spine is always rendered on top of the data lines, which typically have a default zorder of 1-3.