-
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, BUG: Fix errors in IO/loading/projectors #8210
Conversation
Okay, should be all good now @agramfort |
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.
shall we merge this one and try to fix the other readers in other PRs?
@@ -102,7 +102,9 @@ def test_gdf2_data(): | |||
# gh-5604 | |||
assert raw.info['meas_date'] is None | |||
_test_raw_reader(read_raw_gdf, input_fname=gdf2_path + '.gdf', | |||
eog=None, misc=None) | |||
eog=None, misc=None, | |||
test_scaling=False, # XXX this should be True |
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 mean it's not working yet on gdf data?
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.
The values seem too large. Each of these requires some investigation. I think it should be done in another PR. It's possible nothing is broken but it would be good to understand why the EEG values are around 0.01 even after removing the mean or applying an average reference (for example)
@@ -57,7 +57,10 @@ | |||
def test_egi_mff_pause(fname, skip_times, event_times): | |||
"""Test EGI MFF with pauses.""" | |||
with pytest.warns(RuntimeWarning, match='Acquisition skips detected'): | |||
raw = _test_raw_reader(read_raw_egi, input_fname=fname) | |||
raw = _test_raw_reader(read_raw_egi, input_fname=fname, | |||
test_scaling=False, # XXX probably some bug |
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.
idem?
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.
@larsoner merge this one if you want to move on
* upstream/master: (489 commits) MRG, DOC: Fix ICA docstring, add whitening (mne-tools#8227) MRG: Extract measurement date and age for NIRX files (mne-tools#7891) Nihon Kohden EEG file reader WIP (mne-tools#6017) BUG: Fix scaling for src_mri_t in coreg (mne-tools#8223) MRG: Set pyvista as default 3d backend (mne-tools#8220) MRG: Recreate our helmet graphic (mne-tools#8116) [MRG] Adding get_montage for montage to BaseRaw objects (mne-tools#7667) ENH: Allow setting tqdm backend (mne-tools#8177) [MRG, IO] Persyst reader into Raw object (mne-tools#8176) MRG, BUG: Fix errors in IO/loading/projectors (mne-tools#8210) MAINT: vectorize _read_annotations_edf (mne-tools#8214) FIX : events_from_annotation when annotations.orig_time is None and f… (mne-tools#8209) FIX: do not project to sphere; DOC - explain how to get EEGLAB-like topoplots (mne-tools#7455) [MRG, DOC] Added linear algebra of transform to doc (mne-tools#7087) FIX: Travis failure on python3.8.1 (mne-tools#8207) BF: String formatting in exception message (mne-tools#8206) BUG: Fix STC limit bug (mne-tools#8202) MRG, DOC: fix ica tutorial (mne-tools#8175) CSP component order selection (mne-tools#8151) MRG, ENH: Add on_missing to plot_events (mne-tools#8198) ...
* WIP: Expose errors * WIP * WIP * FIX: Almost * FIX: Better test * FIX: Fix bugs * FIX: More lenient
I wanted to add a tiny test to #8176 to ensure
_mult_cal_one
is used properly, but I found out it's not used properly in a number of existing readers. Basically it means (I think) that for all readers for a non-preloadedraw
:raw.apply_proj().load_data().get_data()
was broken (fix WIP)raw.apply_proj().pick(...)
was broken (now disallowed; we didn't allow this until this release cycle so not so bad)_mult_cal_one
was not used properly, so projectors were not applied when data were not preloadedThis sounds bad, but
(1)
hopefully isn't too common a practice, (2) is within-release and probably rare, and (3) affects formats where using projections are less common (and using a patterns involving not preloading that are hopefully uncommon).