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
MRG: Speed up browse figure creation if scalings are provided #10109
Conversation
c0f1b05
to
4aaa522
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm. Thx 🙏 @hoechenberger
mne/viz/utils.py
Outdated
scalings = _handle_default('scalings_plot_raw', scalings) | ||
if not isinstance(inst, (BaseRaw, BaseEpochs)): | ||
raise ValueError('Must supply either Raw or Epochs') | ||
|
||
for key, value in scalings.items(): | ||
if isinstance(value, str) and value != 'auto': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic seems wrong now as the not
was omitted, should probably still be
if isinstance(value, str) and value != 'auto': | |
if not (isinstance(value, str) and value == 'auto'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... if you were trying to distribute the not
, you probably meant:
if isinstance(value, str) and value != 'auto': | |
if not isinstance(value, str) or value != 'auto': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I believe we could probably change this this to
if value != 'auto' and not isinstance(value, float):
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a respective change in cbc14e8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the old conditional better, for example:
>>> import numpy as np
>>> np.array([1.]).item()
1.0
>>> float(np.array([1.]))
1.0
>>> np.array([1.]) == 'auto'
<stdin>:1: FutureWarning: elementwise comparison failed; returning scalar instead, but in the future will perform elementwise comparison
False
That warning would not happen with the isinstance(thing, str)
call version of the code
To me the logic of if not (string and equal to 'auto'): then convert to float
is clearer and more general
@larsoner Please merge if you're happy! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM
mne/viz/utils.py
Outdated
@@ -1158,14 +1158,33 @@ def _compute_scalings(scalings, inst, remove_dc=False, duration=10): | |||
""" | |||
from ..io.base import BaseRaw | |||
from ..epochs import BaseEpochs | |||
|
|||
scalings = deepcopy(scalings) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't need this line. _handle_default
should EDIT: already make a copy internally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(at least if you're worried about the user passing a dict and it being modified, it shouldn't be -- the values in principle could be if they're mutable and modified inplace but in practice this shouldn't really happen I think)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the deepcopy now.
Thanks @hoechenberger ! |
This avoids loading data from disk or performing calculations if we don't actually need to calculate
scalings
parameters.Also adds a
scalings
parameter toReport.add_raw()
.Performance improvements:
with the following code:
x-ref #10107