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

ENH: Write out raw data and SSP events #840

Merged
merged 3 commits into from
Feb 6, 2024
Merged

Conversation

larsoner
Copy link
Member

Before merging …

  • Changelog has been updated (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.

@larsoner
Copy link
Member Author

larsoner commented Jan 31, 2024

All green, ready for review/merge from my end @hoechenberger @agramfort !

I pushed a [ci skip] commit just to remove an errant line from v1.6.md so no need to run CIs again, can just be merged without waiting for requirements if there are no review comments.

@larsoner
Copy link
Member Author

... you can see an example of the Raw (clean) here:

https://output.circle-artifacts.com/output/job/5a50001f-886a-48c7-8e3f-cbcec122019e/artifacts/0/site/examples/ds000248_base/sub-01_task-audiovisual_report.html#Raw__clean___run__01_

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)

Copy link
Member

@drammock drammock left a 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",
Copy link
Member

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.

@larsoner larsoner enabled auto-merge (squash) February 6, 2024 15:34
@larsoner larsoner merged commit 92be603 into mne-tools:main Feb 6, 2024
49 of 50 checks passed
@larsoner larsoner deleted the raw branch February 6, 2024 16:20
@larsoner larsoner mentioned this pull request Feb 6, 2024
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

Successfully merging this pull request may close these issues.

ENH: Preprocessed raw data
2 participants