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

BUG: viz plot window's 'title' argument showed no effect. #12828

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

Conversation

shristibaral
Copy link

viz plot window's 'title' argument showed no effect. Fix:
if title is given, title along with subject's name will be displayed on plot window title, if title is none then subject name will be displayed as title.

Reference issue

Example: Fixes #1234.

What does this implement/fix?

Explain your changes.

Additional information

Any additional information you think is important.

viz plot window's 'title' argument showed no effect.
Fix:
if title is given, title along with subject's name will be displayed on plot window title, if title is none then subject name will be displayed as title.
Copy link

welcome bot commented Sep 5, 2024

Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴

@larsoner
Copy link
Member

larsoner commented Sep 6, 2024

Looks reasonable @shristibaral ! Can you add a doc/changes/devel/12828.bugfix.rst with something like:

Fixed behavior of :func:`mne.viz.plot_source_estimates` where the ``title`` was not displayed properly, by :newcontrib:`Shristi Baral`.

and then add your name to doc/changes/names.inc?

@larsoner larsoner added this to the 1.9 milestone Sep 6, 2024
@shristibaral
Copy link
Author

I have added updated docs file

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Pushed a tiny commit to add title a few more places and document it in the docstrings properly. Will mark for merge-when-green, but might take a couple of hours because we'll want to wait for #12832 to get CIs green first.

Comment on lines +2527 to +2530
if not title:
title = sub_info
else:
title = f"{title}, {sub_info}"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is the right behavior. I don't like that there is no way to prevent subject name from appearing. One concrete case where you don't want to show it: if your subject names aren't de-identified, and you want to show an STC plot in a public presentation/demo.

Most (all?) other places in our codebase, if we have some sort of default/automatic title when title=None, then passing title="whatever" would completely replace the default title. I think we should do likewise here.

Suggested change
if not title:
title = sub_info
else:
title = f"{title}, {sub_info}"
title = title if title is not None else sub_info

Comment on lines +4624 to +4625
Title for the figure and the subject name. If None, only the subject name will be
used.
Copy link
Member

Choose a reason for hiding this comment

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

this controls window title (in the GUI titlebar), not figure title (text on the figure image itself). So this text should be clarified, and also be more clear that the passed title will be prepended to the subject name. However, see other comment below; I'm not convinced that is the right behavior.

Suggested change
Title for the figure and the subject name. If None, only the subject name will be
used.
Title for the figure; will be prepended to the subject name (separated by a comma).
If ``None``, only the subject name will be used.

@larsoner larsoner modified the milestones: 1.9, 1.10 Dec 14, 2024
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.

4 participants