[ENH, MRG] Add mne.viz.Brain.add_head method to display head
#9618
Conversation
alexrockhill
changed the title
mne.viz.Brain.add_head method to display headmne.viz.Brain.add_head method to display head
|
I love all the new brain functionality. But I have had some issues trying to use it (not just this PR, all of it), not due to code issues, but finding appropriate examples. @alexrockhill can you add this new functionality to an existing example? And as a general issue for discussion, would it be appropriate to have a small tutorial that simply works through how to use brain e.g. creating a brain figure, adding some sensors, rotating it, taking a screenshot, marking a region, adding a stc, etc? Maybe this already exists? |
I was going to work on #9594 some time in the near future, maybe that would be a good time to rework a tutorial? Or better yet #8803/#9585. I can open an issue specifically for the tutorials and we can make sure there is ample time to comment on what should be in it. |
|
I see no reason not to start building the example in this PR. Start simple with a brain and then .add_head(), and build as you wish. Thinking further, my understanding is that examples are the more appropriate location for small specialised instructions. So maybe it should go there? |
This is a very small PR so I have no problem adding to it but I think one reason not to start now is that it might be helpful to discuss in the issue how best to implement the example as far as example vs tutorial and what to show so that we can make it easy to find and avoid redundancy. |
|
Good to go I think |
|
@larsoner okay with you? |
larsoner
left a comment
larsoner
left a comment
There was a problem hiding this comment.
You'll notice a theme in my reviews :)
|
Sorry that was a bit sloppy, weird the CIs didn't complain about the docstring omissions. Ok I cleaned it up but I left the docstring because I think the inputs and return type are not at all obvious and I'd like the future me to understand what's going on without having to execute code. |
|
Ready to merge? #9585 depends on this so it would be nice to get moving |
drammock
left a comment
drammock
left a comment
There was a problem hiding this comment.
I've suggested a couple of improvements but overall LGTM.
| color : matplotlib-style color | None | ||
| A list of anything matplotlib accepts: string, RGB, hex, etc. | ||
| (default: "gray"). |
There was a problem hiding this comment.
just a note-to-self that we should consider unifying our "matplotlib-compatible color" docstrings, which are rather heterogeneous at the moment (git grep matplotlib- for a sampling). But not necessary for this PR
|
Ok good to merge, I think. @larsoner might want to review it if you haven't left already since it was your comments I was addressing |
let's see if he gets around to it in the next ~24 hours. If not I'll merge before the weekend; I saw what he had requested before and it looks like you addressed it |
|
Thanks @alexrockhill ! |
5 participants
Fixes #9584.