-
Notifications
You must be signed in to change notification settings - Fork 85
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
When reading FIFF files, "allow Maxshield" by default #788
Conversation
In `main`, when trying to read a FIFF file that was recorded with activated Maxshield and hasn't been Maxwell-filtered yet, one needed to explicitly pass `extra_params=dict(allow_maxshield=True)` to `read_raw_bids()`. However, since we currently only support reading and writing "raw" data properly (i.e., no derivatives), this unncessarily complicates things: raw data is often not Maxwell-filtered, and Maxwell-filtering is one of the very first processing steps after loading the data. In contrast, IF the data was already Maxwell-filtered on the device, this would be clearly recognizible by its `proc` entity, which should indicate such processing. So this commit makes it such that FIFF files will be read with `allow_maxshield=True` by default, unless explicitly disabled via e.g. `extra_params=dict(allow_maxshield=False)`. It also removed the `--allow_maxshield` switch from the `raw_to_bids` command, as this seems similarly superfluous and is not in line with how `write_raw_bids()` handles things.
Codecov Report
@@ Coverage Diff @@
## main #788 +/- ##
=======================================
Coverage 94.19% 94.20%
=======================================
Files 23 23
Lines 2948 2952 +4
=======================================
+ Hits 2777 2781 +4
Misses 171 171
Continue to review full report at Codecov.
|
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 am impartial on this, I you both think it's an improvement I'll trust your choice 👍
It's superfluous now that mne-tools/mne-bids#788 has been merged.
* Drop allow_maxshield config option It's superfluous now that mne-tools/mne-bids#788 has been merged. * Forgot something
A bit late to the party, sorry. |
I think MNE issues a warning nevertheless. Now I'm wondering why we don't see this in our tests 🤔 |
In
main
, when trying to read a FIFF file that was recorded with activated Maxshield and hasn't been Maxwell-filtered yet, one needed to explicitly passextra_params=dict(allow_maxshield=True)
toread_raw_bids()
.However, since we currently only support reading and writing "raw" data properly (i.e., no derivatives), this unncessarily complicates things: raw data is often not Maxwell-filtered, and Maxwell-filtering is one of the very first processing steps after loading the data. In contrast, IF the data was already Maxwell-filtered on the device, this would be clearly recognizable by its
proc
entity, which should indicate such processing, making any further complications just for the sake of "making users aware they need to Maxwell-filter" a bit pointless.So this commit makes it such that FIFF files will be read with
allow_maxshield=True
by default, unless explicitly disabled via e.g.extra_params=dict(allow_maxshield=False)
.I also removed the
--allow_maxshield
switch from theraw_to_bids
command, as this seems similarly superfluous and is not in line with howwrite_raw_bids()
handles things (auto-setsallow_maxshield=True
)Merge checklist
Maintainer, please confirm the following before merging: