Skip to content
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

Add a note on fig.mne.colorbar in the docs of plot_connectivity_circle #266

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

ruuskas
Copy link
Contributor

@ruuskas ruuskas commented Dec 10, 2024

This is related to PR 13019 in MNE-Python and closes #262.

I also tried changing the example mne_inverse_label_connectivity.py to add an example use case. However, it appears that since the figure is rendered before the plot_connectivity_circle function exits, changes made afterwards do not become visible in the figure unless fig.show() is called, which leads to showing the figure twice. Workarounds to prevent this of course exist but might clutter the example unnecessarily. Thoughts?

Copy link
Member

@drammock drammock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @ruuskas, looks good. For this one we'll wait to merge until the upstream change is in and we've formalized what the namespace structure will be like (so that we're certain that fig.mne.colorbar is the correct way to access this)

Marking as "request changes" just as a reminder that we shouldn't merge yet.

@tsbinns tsbinns mentioned this pull request Jan 16, 2025
@drammock
Copy link
Member

I also tried changing the example mne_inverse_label_connectivity.py to add an example use case. However, it appears that since the figure is rendered before the plot_connectivity_circle function exits, changes made afterwards do not become visible in the figure unless fig.show() is called, which leads to showing the figure twice.

I think the right approach in this case is to do plot_connectivity_circle(..., show=False) and then tweak the colorbar and then call fig.show() afterward.

@drammock
Copy link
Member

wait to merge until the upstream change is in and we've formalized what the namespace structure will be like (so that we're certain that fig.mne.colorbar is the correct way to access this)

I don't think we've made any progress on formalizing the fig.mne namespace. It's kind of a big job as it needs to take into account all the possible plot types (at least the matplotlib ones, and maybe the pyvista ones too?). So I'm sorta inclined to just put this one in as fig.mne.colorbar even though I know this may not be ideal since some of our figures have multiple colorbars.

One possible "shortcut" to preventing too much churn later would be to create separate namespaces for each plotting function, so, e.g.: fig.mne.plot_connectivity_circle.colorbar in this case. That way we can incrementally add different plotting functions to the namespace, and hopefully maintain some consistency by looking at what's been done so far whenever it's time to add something new. WDYT?

@ruuskas
Copy link
Contributor Author

ruuskas commented Jan 17, 2025

I don't think we've made any progress on formalizing the fig.mne namespace. It's kind of a big job as it needs to take into account all the possible plot types (at least the matplotlib ones, and maybe the pyvista ones too?). So I'm sorta inclined to just put this one in as fig.mne.colorbar even though I know this may not be ideal since some of our figures have multiple colorbars.

One possible "shortcut" to preventing too much churn later would be to create separate namespaces for each plotting function, so, e.g.: fig.mne.plot_connectivity_circle.colorbar in this case. That way we can incrementally add different plotting functions to the namespace, and hopefully maintain some consistency by looking at what's been done so far whenever it's time to add something new. WDYT?

Is it often the case that e.g. colorbars from multiple plotting functions co-exist inside the same Figure instance? If not, then fig.mne.plot_something.colorbar feels a bit redundant. If a single plotting function call added multiple colorbars, I guess the logical thing would be to put them in a list.

@ruuskas
Copy link
Contributor Author

ruuskas commented Jan 17, 2025

I think the right approach in this case is to do plot_connectivity_circle(..., show=False) and then tweak the colorbar and then call fig.show() afterward.

True! I made a very small addition there.

@ruuskas ruuskas changed the title Add note in doc Add a note on fig.mne.colorbar in the docs of plot_connectivity_circle Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to set colorbar ticks and label in plot_connectivity_circle
3 participants