add initial demo of iterative sphere coloring #11281

timonmerk · 2022-10-25T18:41:22Z

Adresses #11214

What does this implement/fix?

This PR adds the sensor_colors keyword to the following functions:

mne.viz _3d.py plot_alignment()
mne.viz _3d.py _plot_sensors()

Within _plot_sensors() the function _plot_glyphs() is iteratively called to plot and color individually each sphere.

Additional information

An example is demonstrated in tutorials/clinical/30.ecog.py to color individual spheres.

I think there might be potential issues coming up with the actors list in the _3d.py _plot_sensors() function. I appended for each type a list of lists containing each actor. This might be problematic and break code for other scripts that I could not check. _plot_sensors() returns the actor dictionary, but it's not used in plot_alignment(). It would be great to get feedback on that.

drammock

here's some initial feedback. Please also run make flake locally in the repo root, there are some style errors that I didn't comment on that should be fixed.

drammock · 2022-10-25T18:56:34Z

mne/viz/_3d.py

@@ -451,7 +451,7 @@ def plot_alignment(info=None, trans=None, subject=None, subjects_dir=None,
                   meg=None, eeg='original', fwd=None,
                   dig=False, ecog=True, src=None, mri_fiducials=False,
                   bem=None, seeg=True, fnirs=True, show_axes=False, dbs=True,
-                   fig=None, interaction='terrain', verbose=None):
+                   fig=None, interaction='terrain', sensor_colors=None, verbose=None):


any reason not to use colors instead of sensor_colors? Seems simpler.

colors is already used in this function for plotting the surfaces, e.g. line 758, so I wanted to distinguish it from the other keyword.

drammock · 2022-10-25T18:57:46Z

mne/viz/_3d.py

+    sensor_colors: np.array
+        Array with shape (3, num_sensors), default None


Ideally this should support any Matplotlib-compatible color, which means we shouldn't require shape (3, n_sensors). It should be possible to pass, e.g., ['red', 'blue', 'red', 'red', 'green', 'black', ...]

also, I don't think this docstring matches what you're doing in the code... it looks like you expect shape (n_sensors, 3) not (3, n_sensors)?

Thanks for that comment, I will fix it in the next commit

drammock · 2022-10-25T19:00:54Z

mne/viz/_3d.py

@@ -1178,7 +1180,7 @@ def _plot_sensors(renderer, info, to_cf_t, picks, meg, eeg, fnirs,
                  warn_meg, head_surf, units, sensor_opacity=0.8,
                  orient_glyphs=False, scale_by_distance=False,
                  project_points=False, surf=None, check_inside=None,
-                  nearest=None):
+                  nearest=None, sensor_colors: np.array = None):


let's not add type hints for just one param of one function. If we're going to start annotating we should first agree on a policy and approach. That approach might end up being "add them one param at a time as they get touched" but so far no concrete decision / plan is in place so let's not force our hand here.

mne/viz/_3d.py

drammock · 2022-10-25T19:06:45Z

tutorials/clinical/30_ecog.py

@@ -115,9 +115,11 @@
 # (along with xy positions of each electrode in the image), so that later
 # we can plot frequency band power on top of it.

+sensor_colors = np.random.random([len(raw.info["ch_names"]), 3])


adding random colors is not really appropriate for a tutorial (which should generally show best practices, or at least sensible choices that most users would likely want to make). Can you instead demonstrate highlighting one or a few sensors in one or a few colors in order to make a particular point about the data?

drammock · 2022-10-25T19:08:19Z

oops, sorry, I missed the fact that this was a draft PR and reviewed early! (Thanks for taking this on, by the way 🚀)

Co-authored-by: Daniel McCloy <[email protected]>

timonmerk · 2022-10-26T02:15:59Z

@drammock Thanks a lot for the code review. I adapted the changes and added a small demo using the evoked gamma power as an example use for coloring the sensor spheres in 30_ecog.py

drammock

nice improvements! A few more comments, then let's focus on getting the tests to pass. LMK if you need a refresher on how to run the tests locally and debug.

mne/viz/_3d.py

drammock · 2022-10-26T14:29:51Z

mne/viz/_3d.py

+                    else:
+                        color = sensor_colors[idx_sen, :]
+                actor, _ = _plot_glyphs(renderer=renderer,
+                                        loc=(sens_loc * scalar)[idx_sen, :],


some tests are failing with IndexError on this line. Can you figure out why? Offhand guess is that you're not accounting for bad channels, but that's just a guess.

@drammock It would be helpful to know how to run the specific test. E.g. I see that the test test_notebook_alignment() in mne.viz._brain.tests fails due to an index error. How can I run this single test?

And is there optimally also an option for debugging within this specific test? When I put a breakpoint in VSCode in the specific line and try to debug, it won't jump to the breakpoint and does not seem to debug it correctly.

I tried to run pytest mne/viz/_brain/tests/test_notebook.py but it just skipped the test since it "Requires testing data".

Sorry for not being so proficient about using the testing framework. But I am very happy to learn more about it.

Ok, I read the documentation and installed the testing data. It is for me nevertheless confusing that I could not debug within the test:

So I can run the tests now, but VSCode does not stop at the breakpoint.
I tried to set the launch.json accordingly https://stackoverflow.com/a/60678160 and read through a related issue microsoft/vscode-python#693 but was not able to fix it.

Debugging would be probably quite necessary for this type of issue.

to run just one test and automatically enter debugger on error you can do pytest --pdb path/to/test/file.py -k name_of_test_function

tutorials/clinical/30_ecog.py

Co-authored-by: Daniel McCloy <[email protected]>

larsoner · 2022-10-27T22:59:46Z

To run tests and have them debug on failure you can do from the command-line pytest mne/tests/test_whatever.py -k whatever_name --pdb for example. And then to stop at some specific point, you can do 1/0 or raise RuntimeError or whatever. It's not as clean as debug points directly in vscode but it works

timonmerk · 2022-11-05T20:20:23Z

@drammock I had a look at the failing tests, and changed the code in such way that if the sensor_colors keyword is None, the code run's as before. In case it is not None I am iteratively plotting each sensor.
The tests should now pass except for the docstring test. I am not exactly sure how that one can be fixed though.

drammock · 2022-11-05T20:57:33Z

Thanks! I will take a look next week. -------- Original Message --------

…

On Nov 5, 2022, 15:20, timonmerk wrote: ***@***.***(https://github.com/drammock) I had a look at the failing tests, and changed the code in such way that if the sensor_colors keyword is None, the code run's as before. In case it is not None I am iteratively plotting each sensor. The tests should now pass except for the docstring test. I am not exactly sure how that one can be fixed though. — Reply to this email directly, [view it on GitHub](#11281 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AAN2AU47SPHSNTIIVPSNKL3WG26RFANCNFSM6AAAAAAROH4MMA). You are receiving this because you were mentioned.Message ID: ***@***.***>

mne/viz/_3d.py

tutorials/clinical/30_ecog.py

drammock · 2022-11-09T17:47:42Z

mne/viz/_3d.py

+            else:
+                actor_list = []
+                for idx_sen in range(sens_loc.shape[0]):
+                    if isinstance(sensor_colors, list):


this check is still fragile / too stringent. a tuple of color strings will fail this check, and will then error out on the else clause. I suggest something like this:

sensor_colors = np.asarray(sensor_colors) if sensor_colors.ndim not in (1, 2) or sensor_colors.shape[0] != sens_loc.shape[0]: raise ValueError('insert error message about expected shape and actual shape we got') color = sensor_colors[idx_sen] # ...then the call to _plot_glyphs()

Thanks for the review! I improved the error message in my most recent commit.

Co-authored-by: Daniel McCloy <[email protected]>

drammock · 2022-11-15T22:55:01Z

@timonmerk can you merge in current main here? There are some CI failures here that I think have already been fixed in main.

drammock

LGTM! just two nitpicks about the error message, and about avoiding backslash-line-continuation pattern (we prefer parentheses instead).

mne/viz/_3d.py

Co-authored-by: Daniel McCloy <[email protected]>

drammock · 2022-11-17T23:02:04Z

@timonmerk FYI the failing -pre tests are the result of matplotlib/matplotlib#24455, waiting to hear back from them on a recommended workaround.

timonmerk · 2022-11-17T23:33:22Z

@drammock Thanks for following up on that issue! I was also unsure if that is something we should work on by building a workaround in MNE

drammock · 2022-11-18T17:37:41Z

ok, only remaining CI failures are the MPL-related -pre CIs, so in it goes! Thanks @timonmerk

add initial demo of iterative sphere coloring

24a1f00

drammock requested changes Oct 25, 2022

View reviewed changes

timonmerk and others added 3 commits October 25, 2022 21:38

Update mne/viz/_3d.py

b9a1925

Co-authored-by: Daniel McCloy <[email protected]>

add gamma evoked power tutorial, allow mne colors

6d048c4

add list parameter option for sensor_colors

6653cd9

timonmerk marked this pull request as ready for review October 26, 2022 02:16

drammock reviewed Oct 26, 2022

View reviewed changes

timonmerk and others added 2 commits October 26, 2022 18:34

Update mne/viz/_3d.py

41952df

Co-authored-by: Daniel McCloy <[email protected]>

Update mne/viz/_3d.py

78c5a3e

Co-authored-by: Daniel McCloy <[email protected]>

timonmerk added 3 commits November 5, 2022 13:19

fix iteration for individual sensor plots

56a4616

fix sensor type being source

cafa6f8

make flake pass

735ff03

drammock reviewed Nov 8, 2022

View reviewed changes

mne/viz/_3d.py Outdated Show resolved Hide resolved

docstring whitespace fix

006e88c

drammock reviewed Nov 9, 2022

View reviewed changes

timonmerk and others added 4 commits November 10, 2022 20:40

Update tutorials/clinical/30_ecog.py

0df3a22

Co-authored-by: Daniel McCloy <[email protected]>

Update tutorials/clinical/30_ecog.py

d8e07cf

Co-authored-by: Daniel McCloy <[email protected]>

improve example; add wrong shape exception

acb0d15

fix line length

9a6703e

drammock approved these changes Nov 15, 2022

View reviewed changes

mne/viz/_3d.py Outdated Show resolved Hide resolved

mne/viz/_3d.py Outdated Show resolved Hide resolved

timonmerk and others added 3 commits November 16, 2022 22:48

Update mne/viz/_3d.py

af2cfe4

Co-authored-by: Daniel McCloy <[email protected]>

Update mne/viz/_3d.py

d314034

Co-authored-by: Daniel McCloy <[email protected]>

merge upstream main

cf2c6b0

Merge branch 'main' into color_sensors

84ec8ab

drammock merged commit ad4c99c into mne-tools:main Nov 18, 2022

		sensor_colors: np.array
		Array with shape (3, num_sensors), default None

add initial demo of iterative sphere coloring #11281

add initial demo of iterative sphere coloring #11281

Uh oh!

Conversation

timonmerk commented Oct 25, 2022

What does this implement/fix?

Additional information

Uh oh!

drammock left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drammock commented Oct 25, 2022

Uh oh!

timonmerk commented Oct 26, 2022

Uh oh!

drammock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

larsoner commented Oct 27, 2022

Uh oh!

timonmerk commented Nov 5, 2022

Uh oh!

drammock commented Nov 5, 2022 via email

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drammock commented Nov 15, 2022

Uh oh!

drammock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

drammock commented Nov 17, 2022

Uh oh!

timonmerk commented Nov 17, 2022

Uh oh!

drammock commented Nov 18, 2022

Uh oh!

Uh oh!