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

Fix report section on original raw data (didn't contain bad channels and subject or experimenter name) #931

Merged
merged 9 commits into from
Apr 23, 2024

Conversation

hoechenberger
Copy link
Member

@hoechenberger hoechenberger commented Apr 23, 2024

Fixes #930

Before merging …

  • Changelog has been updated (docs/source/changes.md)

@hoechenberger
Copy link
Member Author

hoechenberger commented Apr 23, 2024

@larsoner This seems to fix my problem

I only have one question

Should we only set the pre-existing bads when generating the HTML repr for the original data, or should we set the pre-existing bads + all bads from the data quality checks?

Either approach seems to have its up- and downsides.

cc @SophieHerbst

@@ -848,9 +848,9 @@ def _add_raw(
cfg: SimpleNamespace,
report: mne.report.Report,
bids_path_in: BIDSPath,
raw: BaseRaw,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change should avoid the problem we saw here, as it now forces us to provide pre-loaded raw data (with hopefully the correct metadata set). previously this was optional. see also the changes below

@hoechenberger
Copy link
Member Author

Circle fails for ds000246 due to outdated cache

@hoechenberger hoechenberger marked this pull request as ready for review April 23, 2024 13:13
@larsoner
Copy link
Member

I'll fix the caches in #929

@hoechenberger
Copy link
Member Author

I'll fix the caches in #929

Thank you!

@larsoner
Copy link
Member

Should we only set the pre-existing bads when generating the HTML repr for the original data, or should we set the pre-existing bads + all bads from the data quality checks?

To me the optimal thing to do here I think is to have two report entries:

  1. One which is the data as it comes originally from the dataset author with bads from the sidecar. It tells you what you start with before doing anything to it with the pipeline
  2. Another which is the data after our autobads (this section only needs to exist if we do some autobad detection). It tells you what the automated approaches did to the data

To me it's okay for brevity to have (1) have all the nice raw data plots, and have (2) maybe have less -- like just a list of channels removed or something? I think we're already kind of close to this organization:

image

But from that data quality plot it's not immediately clear which channels were added as bad, if any. Even just a HTML entry that lists the bads (when bad enabled) and flats (when flat enabled) would be a nice help

@larsoner
Copy link
Member

... and organizationally I think it might make sense to call that section in the sidebar Raw (autobad) or similar. It makes it clear the parentheticals from original->autobad->maxwell->filtered->cleaned. "Data Quality" is an outlier at the moment in that it uses the step name

@hoechenberger
Copy link
Member Author

hoechenberger commented Apr 23, 2024

Thanks for sharing your ideas, they resonate well with me!

I believe then at least the bits of code that this PR touches should be good! And the rest could be addressed in future PRs

I will add a changelog entry and once CI passes, we should be good to go.

@larsoner
Copy link
Member

I merged main into the branch, you should be able to add to the changelog now and it should come back green

@hoechenberger
Copy link
Member Author

Thanks @larsoner, I pushed changelog entries and enabled auto-merge

@hoechenberger hoechenberger merged commit a52c00c into mne-tools:main Apr 23, 2024
51 of 52 checks passed
@hoechenberger hoechenberger deleted the hoechenberger/issue930 branch April 23, 2024 15:32
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.

Report section on original raw data doesn't contain bads
2 participants