MAINT: Remove _get_eeg_calibration_info and delegate GCAL scaling to mffpy#14011
Merged
scott-huberty merged 7 commits intoJul 2, 2026
Merged
Conversation
17 tasks
PragnyaKhandelwal
requested review from
agramfort,
drammock and
larsoner
as code owners
Contributor
Author
|
test failure seems unrelated.... |
| logger.info(" Reading events ...") | ||
| egi_events, egi_info, mff_events = _read_events(input_fname, egi_info) | ||
| cals = _get_eeg_calibration_info(input_fname, egi_info) | ||
| _cal_scales = {"uV": 1e-6, "V": 1} |
Contributor
There was a problem hiding this comment.
Total nitpick but.. This variable is defined in two places (_read_evoked_mff, RawMff). You could move it to the module level as a constant. Unfortunately frozendict was not introduced until Python 3.15, so you can just keep it as a regular, mutable dict 🙂
Contributor
The CircleCI jobs failed because the pre-commit bot made a commit after you, and the CIrcleCI jobs won't run on commits from that bot. |
| ) | ||
|
|
||
| REFERENCE_NAMES = ("VREF", "Vertex Reference") | ||
| _cal_scales = {"uV": 1e-6, "V": 1} |
Contributor
There was a problem hiding this comment.
Suggested change
| _cal_scales = {"uV": 1e-6, "V": 1} | |
| CAL_SCALES = {"uV": 1e-6, "V": 1} |
As mentioned on our call today, there is a convention to make constants upper-case, as a hint to other developers that this variable should not be mutated.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reference issue (if any)
Part of #13926 (Phase 1 — legacy helper cleanup).
Follows #14007.
What does this implement/fix?
Removes
_get_eeg_calibration_infofromegimff.pyand letsmffpyapply per-channel GCAL scaling natively instead.Previously,
_get_mff_readerdisabledmffpy's built-in GCAL calibration (set_calibration("EEG", None)) because_get_eeg_calibration_infowas already baking GCAL into MNE'scalsarray. This split the calibration responsibility across two places. Nowmffpyhandles GCAL directly (as it does by default), and MNE's cals for EEG channels are simplified to a flat unit conversion (1e-6 for µV channels, 1 for V channels).As a result,
_get_gainsingeneral.pyloses its only caller and is removed entirely.Additional information
The
value_range/bitsbranch inside_get_eeg_calibration_infowas dead code — both fields are hardcoded to 0 in_read_headerand never updated.