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

BUG: Fix several bugs #839

Merged
merged 10 commits into from
Jan 30, 2024
Merged

BUG: Fix several bugs #839

merged 10 commits into from
Jan 30, 2024

Conversation

larsoner
Copy link
Member

@larsoner larsoner commented Jan 30, 2024

Before merging …

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

I went to work on #831 but wanted to sort out a few little other bugs (and one minor enhancement) first:

  1. Fix bug where using mf_reference_run = None could change the empty room file matched from run-to-run of mne_bids_pipeline due to use of set, whose order is not guaranteed. @hoechenberger I suspect this could be the actual root cause of Caching seems not to be working on macOS #814

  2. Output the reject params in the reject step -- though this can be set in the config, it's sometimes calculated on-the-fly by autoreject and not saved anywhere. Even in the manual dict case it's nice to have it near the drop logs in the report in case you want to think about adjusting your thresholds:

    Screenshot from 2024-01-30 12-00-44

  3. Fix bug where reject was getting restricted incorrectly for M/EEG channels

  4. Prefer stat to lstat because I'm pretty sure we don't want to use the symlink's own mtime but rather the mtime of whatever it points to. (This is apparently the only difference between lstat and stat.) Doubt anyone has actually hit this so no changelog needed, but better to have it in there I think.

  5. Fix bug with -regress saving where instead of writing one for each run, a single filename was written to by every run (within-release change so no changelog needed).

  6. Slightly better report tags for the cleaned epochs to make it clear that what's happened is the reject step

@hoechenberger
Copy link
Member

Thanks @larsoner!

I think the value of the "reject" parameter should be included via upstream changes to mne.Report.add_epochs(). But we can merge here and move the feature to MNE-Python at a later time. Would be good to create an issue as a reminder

@larsoner
Copy link
Member Author

Found another bug! --no-cache didn't work. Pushing now. Also nesting the rejection threshold stuff under the Epochs: after cleaning section of the report. Marking for merge-when-green but let me know if you'd like me to hold off @hoechenberger !

@larsoner larsoner enabled auto-merge (squash) January 30, 2024 17:14
@larsoner larsoner merged commit a65f278 into mne-tools:main Jan 30, 2024
51 of 52 checks passed
@larsoner larsoner deleted the raw branch January 30, 2024 19:37
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.

2 participants