Skip to content
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

Assertion error if raw has HP/LP set, but config specifies lower/higher HP/LP #1008

Open
skjerns opened this issue Oct 24, 2024 · 2 comments

Comments

@skjerns
Copy link

skjerns commented Oct 24, 2024

Our MEG by default records with HP=0.1 and LP=330.

In my config.conf, I have mistakenly set l_freq=0.05, which now raises an AssertionError as the filtering will not work.

Probably there should be a check if the requested highpass/lowpass are actually feasable?

l_freq: float = 0.05  # Apply high-pass filter at 0.1 Hz
h_freq: Optional[float] = None  # Disable low-pass filter

console output:

┌────────┬ preprocessing/_06a1_fit_ica ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
│17:54:36│ ⏳️ sub-01 Processing raw data from sub-01_task-main_proc-filt_raw.fif
│17:54:37│  sub-01 A critical error occurred. The error message was: 

Starting post-mortem debugger.
<traceback object at 0x7fcaa857a880>
> /zi/home/simon.kern/anaconda3/lib/python3.11/site-packages/mne_bids_pipeline/steps/preprocessing/_06a1_fit_ica.py(104)run_ica()
-> assert np.allclose(raw.info["highpass"], cfg.l_freq)
# ->> I figured that the recording MEG sets a highpass of 0.1, so my 0.05 don't have much effect. `raw['highpass'] is indeed 0.1, while `cfg.l_freq=0.05`
Copy link

welcome bot commented Oct 24, 2024

Hello! 👋 Thanks for opening your first issue here! ❤️ We will try to get back to you soon. 🚴🏽‍♂️

@skjerns skjerns changed the title Assertion error if raw has HP/LP set, but config specifies differently Assertion error if raw has HP/LP set, but config specifies lower/higher HP/LP Oct 24, 2024
@larsoner
Copy link
Member

Yeah it might be better to check if the bound is less than the current info["highpass"]. Safest would be to make it an error if it is. We could be more permissive and make it a a warning, though, in case you want more attenuation than DAQ highpass provides. But in either case I agree that the current AssertionError is not super helpful!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants