[MRG] Do not plot duplicate labels in montage#4592
[MRG] Do not plot duplicate labels in montage#4592larsoner merged 6 commits intomne-tools:masterfrom
Conversation
|
Can someone please start AppVeyor? |
mne/viz/montage.py
Outdated
| """Functions to plot EEG sensor montages or digitizer montages.""" | ||
| from ..utils import check_version | ||
| import numpy as np | ||
| from scipy.spatial.distance import cdist |
There was a problem hiding this comment.
this import will need to be nested (to avoid import mne slowdown we nest most of the scipy imports)
mne/viz/montage.py
Outdated
| logger.info(", ".join([ch_names[d[0]] + "/" + ch_names[d[1]] | ||
| for d in dupes])) | ||
| logger.info("Plotting {} unique labels.".format(n_chans - n_dupes)) | ||
| montage.ch_names = [montage.ch_names[i] for i in idx] |
There was a problem hiding this comment.
You should make a copy of the montage before modifying it
| ch_names = montage.ch_names | ||
| montage.pos = montage.pos[idx, :] | ||
| montage.selection = np.arange(n_chans - n_dupes) | ||
|
|
There was a problem hiding this comment.
Is there some simple way to add a test? Check the number of points in some matplotlib object when plotting the montage with dups?
|
I don't know why AppVeyor isn't running, I'm not sure of a way to manually trigger it. But it will hopefully run on your next commit. (Even if it doesn't there isn't anything Windows-specific here so I think we'll be okay) |
|
Regarding the test, I don't think that we need to validate this against the plot. We want to test if the number of duplicates is detected correctly, right? A very simple test would be to check some montage files where we know the number of duplicates. Is this too simplistic? |
|
I don't see any errors in the CircleCI log file. This line returns a non-zero exit code: Did anything change in the build tools that causes the grep to fail? |
|
No, if you look at the log just above that one you'll see it's a link-parsing error in SG. There is a fix on the way to SG for that. |
|
I see - weird that it still returns 0 as its exit status. So this means that we can ignore CircleCI until this is fixed, right? Regarding this PR, how do we handle |
|
I wouldn't bother for |
|
(and for CircleCI, we just currently make sure the error we get is the SG error and not something else) |
Codecov Report
@@ Coverage Diff @@
## master #4592 +/- ##
==========================================
+ Coverage 87.05% 87.05% +<.01%
==========================================
Files 349 349
Lines 66080 66108 +28
Branches 10256 10259 +3
==========================================
+ Hits 57525 57553 +28
Misses 5657 5657
Partials 2898 2898 |
|
What about the test (see my comment above)? |
mne/viz/montage.py
Outdated
| The figure object. | ||
| """ | ||
| from copy import deepcopy | ||
| import numpy as np |
There was a problem hiding this comment.
don't nest imports of numpy and copy
| montage.ch_names = [montage.ch_names[i] for i in idx] | ||
| ch_names = montage.ch_names | ||
| montage.pos = montage.pos[idx, :] | ||
| montage.selection = np.arange(n_chans - n_dupes) |
There was a problem hiding this comment.
is these lines are not covered by any test then indeed with need a test
|
@cbrnr you need to rebase |
89fd0e8 to
ebeb4c4
Compare
|
Done. Let me know if you think the test is adequate. |
| def test_plot_montage(): | ||
| """Test plotting montages.""" | ||
| """Test plotting montages. | ||
| """ |
There was a problem hiding this comment.
This is going the wrong direction -- single-line docstrings should end with .""" according to pydocstyle
|
Thanks @cbrnr will fix the docstring nitpicks later (we have them set to be ignored by |
Fix #4403.