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

Use matplotlib-based raw browser in docs #10481

Closed
cbrnr opened this issue Apr 1, 2022 · 5 comments · Fixed by #10501
Closed

Use matplotlib-based raw browser in docs #10481

cbrnr opened this issue Apr 1, 2022 · 5 comments · Fixed by #10501
Labels

Comments

@cbrnr
Copy link
Contributor

cbrnr commented Apr 1, 2022

I think that showing raw plots in our docs should not include toolbar icons, scrollbars, and other interactive (irrelevant) items, as is the case since 1.0 (e.g. the screenshot in https://mne.tools/dev/auto_examples/io/read_xdf.html).

I would switch to the matplotlib backend in our docs to get rid of UI elements, unless it is also possible to do this with the qt backend (zen mode maybe?).

@cbrnr cbrnr added the DOC label Apr 1, 2022
@larsoner
Copy link
Member

larsoner commented Apr 1, 2022

I would switch to the matplotlib backend in our docs

Looks like you missed #9960 and #10359 where we worked out using the Qt backend in the doc build, since this sounds like an argument to undo most of that. Given that people at the time at least were more or less "pro" this change IIRC, I'd assume they're -1 on reverting it now.

unless it is also possible to do this with the qt backend (zen mode maybe?).

In principle we could add zen-mode-style capture. However, even back when we used matplotlib we included the UI elements like scrollbars, e.g. here. Personally I don't mind having them in there because it's a bit closer to the user experience and doesn't take up too much space (unlike in Brain where the UI elements arguably do, and we do not capture them). Also, the scrollbars do convey implicit information (e.g., number of channels). So I'm -1 on a full "zen mode" where we remove all interactive elements including scrollbars and overview mode (not sure if that was included in your concept or not).

We could consider individual elements:

  1. The toolbar doesn't add information and might be easy to hide, so we could do that.
  2. Scrollbars are small, so removing them isn't a huge gain. We could probably setVisible(False) them but I don't think it will change much (and again, loses a bit of information).
  3. The overview bar is at least sometimes helpful/informative, so it's nice to have it in there. In individual examples/tutorials we could also switch it to z-score mode if it would help. So I'd rather not remove that.
  4. "Overview bar mode" selector -- actually I wonder if we should move this up to the toolbar (@marsipu ?). It's a better use of vertical space, even though it moves it away from the overview. Given how rarely it's changed, it seems worth it. I'll open a mne-qt-browser issue to discuss separately...

I guess we could hide the toolbar and overview-mode-selector. The downside is that it creates a mismatch with what the user will see, but I guess it's an understandable one. I'm +/-0 on this.

@cbrnr
Copy link
Contributor Author

cbrnr commented Apr 1, 2022

Looks like you missed #9960 and #10359 where we worked out using the Qt backend in the doc build, since this sounds like an argument to undo most of that.

Seems like I did!

I'm OK with not hiding everything, but at least the toolbar and the overview bar do not add any important information. If they do in a particular example, we could explicitly enable them, but by default I'd hide them.

Usually, we want to show the continuous signals in the tutorials (even in the example you mentioned), and we don't need the scrollbars or anything else.

So if the backend cannot be reverted, I'd vote for a zen-mode-like display (no overview bar, no toolbar, and even no scrollbars, although I'd be OK with leaving those in).

@larsoner
Copy link
Member

larsoner commented Apr 4, 2022

So things to improve, assuming we stick with Qt (which I think we should, since it's our default):

  1. Complete the scraper. There are places like the ERP tutorial where we `with use_browser_backend('matplotlib')', e.g., to get the projector dialog scraped. I'm happy to add support for this (shouldn't be too difficult I think!)
  2. Maybe add overview_mode=... to raw.plot etc. if users don't find it that helpful
  3. Decide if we want to change the default for overview_mode:
    1. Not at all (leave it "on" everywhere as it is now)
    2. Just in the doc build (advantage: better view of signals sometimes, disadvantage: deviates from the user experience, potentially less information when annot/events present)
    3. For users (disable by default)

Currently, the overview bar is always shown. One advantage to it for users -- in addition to being a source of information about annot/events/zscore -- is that you can interact with it to get to a location in the channel-time plane. Considering this, I think my +1 goes to (for simplicity) not bothering to add overview_mode for now, and instead change the scraper.

We can go either 1) opt-in where the overview bark isn't scraped unless a "# sphinx_gallery_qt_browser_overview_channels/_zscore is in the code block, or 2) opt-out where it's shown unless a # sphinx_gallery_qt_browser_overview_off is in the code block. I'm not sure if opt-in or opt-out is better...

@cbrnr
Copy link
Contributor Author

cbrnr commented Apr 4, 2022

I'd love an overview_mode=... argument. I think we should expose all browser settings as arguments as a general rule.

I would leave the overview bar on for users, because it adds helpful information. However, I think the overview bar should be opt-in in the docs. Usually, we want to show the raw signals and not so much the exact interactive interface. If this is the goal, we can enable the bar explicitly.

I'd actually prefer the argument over the scraper, because I think it might be useful for people to open the raw browser with their preferred overview bar mode. This could also be a setting we could store in the config.

@larsoner
Copy link
Member

larsoner commented Apr 4, 2022

That would end up being pretty easy implementation-wise, too, because we can just export MNE_BROWSER_OVERVIEW_MODE=off or false or whatever in CircleCI. I can start on an implementation for this in the coming days

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants