Update mnefun._reports to work with current mne-python #364

nordme · 2023-03-31T19:41:16Z

I think good_hpi_count, raw_segments, and psd under mnefun._reports could all use some help.

I'm starting with tossing the the mne.fixes call from good_hpi_count (time_kwargs is unused).

codecov-commenter · 2023-03-31T19:45:01Z

Codecov Report

Merging #364 (0224406) into main (dedf9fa) will decrease coverage by 0.01%.
The diff coverage is 5.00%.

@@            Coverage Diff             @@
##             main     #364      +/-   ##
==========================================
- Coverage   19.99%   19.99%   -0.01%     
==========================================
  Files          33       33              
  Lines        5161     5167       +6     
==========================================
+ Hits         1032     1033       +1     
- Misses       4129     4134       +5

nordme · 2023-03-31T21:57:45Z

Ok, reports are working for me now except for raw_segments. On my machine, raw.plot() gives me a MNEQtBrowser instance, but mnefun/_reports seems to expect a matplotlib instance. I'm turning off this section for running reports for today, but let's chat about fixing it.

drammock · 2023-03-31T22:56:21Z

Ok, reports are working for me now except for raw_segments. On my machine, raw.plot() gives me a MNEQtBrowser instance, but mnefun/_reports seems to expect a matplotlib instance.

could use a context handler with mne.viz.use_browser_backend('matplotlib'): or could set it for the session duration with mne.viz.set_browser_backend('matplotlib'). For changes within mnefun code I'd guess the context handler is better?

nordme · 2023-03-31T23:19:32Z

One more issue. It looks like SVG as an image format option isn't working for me for some reason. The drop_log plot uses SVG as the image format by default in mnefun (and looks like the only section to do so); it works if I change the image format to png, but otherwise not. Here's what my drop log plot looks like. lol!!

nordme · 2023-03-31T23:34:06Z

@drammock Strangely, it looks like _report_raw_segments is run under a context handler already that should be using matplotlib. I haven't seen an obvious reason why it isn't working for me yet.

nordme · 2023-04-01T01:00:01Z

Ok, I figured out the raw_segments issue. Aside from context management, you can't pass a backend choice to raw.plot() directly; you can only set it in your mne-python.json configuration file on your own machine. MNE_BROWSER_BACKEND wasn't set in my config file, and the automatic backend selection function for mne returns qt because it checks if qt works before checking if matplotlib works.

Because there's already a context management function defined for mnefun reports, I think it makes sense to call mne.viz.set_browser_backend('matplotlib') within the report_context() function but let me know if that's a bad idea for some reason.

larsoner · 2023-04-01T17:43:08Z

mnefun/_report.py

@@ -40,6 +40,7 @@
 @contextmanager
 def report_context():
    """Create a context for making plt and mlab figures."""
+    mne.viz.set_browser_backend('matplotlib')


you should use the https://mne.tools/dev/generated/mne.viz.use_browser_backend.html context manager so that it is only set temporarily

So, I tried use_browser_backend first, but that failed for me (I still got a qt instance). :) I didn't dig in to why it wasn't working -- thoughts?

Did you do it in a "with" clause? It won't work if it's not done that way. Even though it's within another context manager, create a second context within it for the browser backend.

Specifying my above comment: I think I had trouble because I don't have experience nesting context managers inside of other context managers. I can see it's possible because in the report_context() function, you do this:

try: with plt.style.context(style): yield except Exception: matplotlib.use(old_backend, force=True) plt.interactive(is_interactive) raise

to use the plt style context. But what's the functional and proper way to do that embedding with mne.viz.use_browser_backend ? Then I was having doubts about how many levels of context nesting make sense! But I do see the advisability of only setting the backend temporarily.

You can do as many levels you want

with a: with b: ...

Or if you will want them both active the whole .. of code (nothing with just a) then you can instead do

with a, b: ...

Without looking at the code... I think in report_context we probably set the backend to Agg, which is a non-GUI backend. So nothing popping up is the correct/desired behavior here!

Alrighty, friends, I propose the following fix. Basically, I think my issue is Pycharm, which tries to do some backend handling stuff when importing matplotlib. If I call with mne.viz.use_backend_browser('matplotlib'): after importing matplotlib and forcing it to use Agg in the report_context() function, then everything works.

@larsoner (Sorry, I didn't see your comment earlier). Agg shouldn't cause your console to freeze such that you have to force quit it though, right?? 😆 I ran the above code from interactive python in a terminal earlier today, and all of the above code options worked perfectly from the terminal, so that makes me feel pretty confident that Pycharm is what's causing my problems!!

🌩️#️⃣⁉️pycharm💩

@drammock You know, you were the one who told me back in the day that checking your code in the terminal is a good troubleshooting step. Thanks for the good advice!!! 😆

larsoner

@drammock feel free to merge if you're happy

nordme · 2023-04-03T20:39:45Z

So, I propose that we just use the png format for all our mnefun plots. It's immediately functional, and we can work on the mne.reports functionality separately.

nordme · 2023-04-03T20:46:59Z

Ok, reports are running smoothly both with my genz2 environment and with a fresh dev environment running mne 1.4, so we're in business on my end!

larsoner · 2023-04-03T20:47:36Z

Sounds good except that we should use webp instead of png nowadays. It produces smaller files even at lossless quality

nordme · 2023-04-03T20:48:37Z

Mmm, ok. How much backward compatibility do we have with webp?

larsoner · 2023-04-03T20:56:08Z

Based on docstring from https://mne.tools/dev/generated/mne.Report.html it looks like 1.3 which is probably new enough. But you could do a version check to decide if you want to be more backward compatible

nordme · 2023-04-03T22:34:56Z

Ok, mne.Report does its own checking to see if it can use webp, so my thought was to just use whatever it comes up with when it instantiates to control all the rest of the report building beneath that.

drammock

circle is complaining about missing config / oauth change. @larsoner do you know what that's about?

mnefun/_report.py

remove mne.fixes

5767188

nordme added 3 commits March 31, 2023 13:54

psd fix

425fd56

flake fix

e7a1ee7

fix mne-python version check for flake

c100cdc

use matplotlib backend

bb9b0f5

larsoner reviewed Apr 1, 2023

View reviewed changes

temporarily set backend

935448b

larsoner approved these changes Apr 3, 2023

View reviewed changes

nordme added 2 commits April 3, 2023 12:09

1.4 fixes: posix path and vlim arg format

aa259c3

use png

0224406

set default image format using mne.Report attribute

2ab3c9c

drammock approved these changes Apr 4, 2023

View reviewed changes

mnefun/_report.py Outdated Show resolved Hide resolved

drammock reviewed Apr 4, 2023

View reviewed changes

mnefun/_report.py Outdated Show resolved Hide resolved

Apply suggestions from code review

bad12c6

drammock reviewed Apr 4, 2023

View reviewed changes

mnefun/_report.py Outdated Show resolved Hide resolved

fix whitespace

73a3a3c

drammock enabled auto-merge (squash) April 4, 2023 14:22

drammock merged commit df17487 into LABSN:main Apr 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update mnefun._reports to work with current mne-python #364

Update mnefun._reports to work with current mne-python #364

nordme commented Mar 31, 2023

codecov-commenter commented Mar 31, 2023 •

edited

Loading

nordme commented Mar 31, 2023

drammock commented Mar 31, 2023

nordme commented Mar 31, 2023 •

edited

Loading

nordme commented Mar 31, 2023

nordme commented Apr 1, 2023 •

edited

Loading

larsoner Apr 1, 2023

nordme Apr 2, 2023

drammock Apr 2, 2023

nordme Apr 2, 2023 •

edited

Loading

larsoner Apr 2, 2023

larsoner Apr 2, 2023

nordme Apr 2, 2023

nordme Apr 3, 2023

drammock Apr 3, 2023

nordme Apr 3, 2023

larsoner left a comment

nordme commented Apr 3, 2023

nordme commented Apr 3, 2023

larsoner commented Apr 3, 2023

nordme commented Apr 3, 2023

larsoner commented Apr 3, 2023

nordme commented Apr 3, 2023

drammock left a comment

Update mnefun._reports to work with current mne-python #364

Update mnefun._reports to work with current mne-python #364

Conversation

nordme commented Mar 31, 2023

codecov-commenter commented Mar 31, 2023 • edited Loading

Codecov Report

nordme commented Mar 31, 2023

drammock commented Mar 31, 2023

nordme commented Mar 31, 2023 • edited Loading

nordme commented Mar 31, 2023

nordme commented Apr 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nordme Apr 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larsoner left a comment

Choose a reason for hiding this comment

nordme commented Apr 3, 2023

nordme commented Apr 3, 2023

larsoner commented Apr 3, 2023

nordme commented Apr 3, 2023

larsoner commented Apr 3, 2023

nordme commented Apr 3, 2023

drammock left a comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 31, 2023 •

edited

Loading

nordme commented Mar 31, 2023 •

edited

Loading

nordme commented Apr 1, 2023 •

edited

Loading

nordme Apr 2, 2023 •

edited

Loading