-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Changes from 2 commits
4aaa522
3a209df
f9d2f6f
cbc14e8
b86da7f
08edb7c
66e64e2
7475eab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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) | ||||||||||
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 commentThe reason will be displayed to describe this comment to others. Learn more. This logic seems wrong now as the
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ... if you were trying to distribute the
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. I like the old conditional better, for example:
That warning would not happen with the To me the logic of |
||||||||||
try: | ||||||||||
scalings[key] = float(value) | ||||||||||
except Exception: | ||||||||||
raise ValueError( | ||||||||||
f'scalings must be "auto" or float, got ' | ||||||||||
f'scalings[{key!r}]={value!r} which could not be ' | ||||||||||
f'converted to float' | ||||||||||
) | ||||||||||
|
||||||||||
# If there are no "auto" scalings, we can return early! | ||||||||||
if all( | ||||||||||
[scalings[ch_type] != 'auto' | ||||||||||
for ch_type in inst.get_channel_types(unique=True)] | ||||||||||
): | ||||||||||
return scalings | ||||||||||
|
||||||||||
ch_types = channel_indices_by_type(inst.info) | ||||||||||
ch_types = {i_type: i_ixs | ||||||||||
for i_type, i_ixs in ch_types.items() if len(i_ixs) != 0} | ||||||||||
scalings = deepcopy(scalings) | ||||||||||
|
||||||||||
if inst.preload is False: | ||||||||||
if isinstance(inst, BaseRaw): | ||||||||||
|
@@ -1191,15 +1210,7 @@ def _compute_scalings(scalings, inst, remove_dc=False, duration=10): | |||||||||
data = inst._data.swapaxes(0, 1).reshape([len(inst.ch_names), -1]) | ||||||||||
# Iterate through ch types and update scaling if ' auto' | ||||||||||
for key, value in scalings.items(): | ||||||||||
if key not in ch_types: | ||||||||||
continue | ||||||||||
if not (isinstance(value, str) and value == 'auto'): | ||||||||||
try: | ||||||||||
scalings[key] = float(value) | ||||||||||
except Exception: | ||||||||||
raise ValueError( | ||||||||||
f'scalings must be "auto" or float, got scalings[{key!r}]=' | ||||||||||
f'{value!r} which could not be converted to float') | ||||||||||
if (key not in ch_types or value != 'auto'): | ||||||||||
hoechenberger marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
continue | ||||||||||
this_data = data[ch_types[key]] | ||||||||||
if remove_dc and (this_data.shape[1] / inst.info["sfreq"] >= duration): | ||||||||||
|
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 internallyThere 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.