-
Notifications
You must be signed in to change notification settings - Fork 67
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
ENH: Write out raw data and SSP events #840
Conversation
All green, ready for review/merge from my end @hoechenberger @agramfort ! I pushed a |
... you can see an example of the I think it's useful enough to have these raw runs for comparison with the original and filtered ones in terms of time traces and PSDs to just have this always on (rather than add an option for it, which was a potential idea in #831) |
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 I took a look as requested. Changes look reasonable (with the caveat that, as you know, I'm not super familiar with this codebase yet). Just one comment.
) -> SimpleNamespace: | ||
cfg = SimpleNamespace( | ||
processing="filt" if config.regress_artifact is None else "regress", |
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.
this is the third time I've seen the same inline "filt-or-regress" if-else. Makes me worry about forgetting to copy it where it's needed, when adding new code. Not sure if there is a sensible way to reduce that cognitive overhead / duplication that wouldn't feel over-engineered, though.
Before merging …
docs/source/changes.md
)Closes #831
Also does some cleanup of
processing=
where we errantly used some uncleaned data -- not just no SSP (which would be taken into account during inversion at least), but no ICA, no regression, and no filtering it seems.