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 bugs with split files #597

Merged
merged 3 commits into from
Sep 8, 2022
Merged

BUG: Fix bugs with split files #597

merged 3 commits into from
Sep 8, 2022

Conversation

larsoner
Copy link
Member

@larsoner larsoner commented Sep 7, 2022

Before merging …

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

Closes #593

True TDD, should fail, then I'll fix it and mark ready for review.

@larsoner
Copy link
Member Author

larsoner commented Sep 7, 2022

@hoechenberger the failure for ds003775 is unexpected:

[15:42:36] preprocessing/_02_frequency_filter sub-010 ses-t1 A critical error occured. The error message was: Unit for channel Fp1 is present in the file. Cannot overwrite it with the units argument.

Looks like it's due to mne-tools/mne-python#11099, can you look?

@hoechenberger
Copy link
Member

Ouch … I'm afraid I won't have time before the weekend, sorry

@larsoner larsoner marked this pull request as ready for review September 7, 2022 17:47
@larsoner
Copy link
Member Author

larsoner commented Sep 7, 2022

This is ready for review/merge from my end

003775 will be red until mne-tools/mne-python#11143 is merged, but hopefully 000248 (where splits matter) and all others come back green. Once 3775 is fixed I'll restart CIs. So whoever reviews, if you're happy, feel free to mark for merge-when green.

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

just a couple of nitpicks

🙏 @larsoner

epochs_cleaned.save(
fname_epo_out, overwrite=True, split_naming='bids',
split_size=cfg._epochs_split_size)
# _update_for_splits(out_files, 'epochs_cleaned')
Copy link
Member

Choose a reason for hiding this comment

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

to cleanup

Copy link
Member Author

Choose a reason for hiding this comment

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

These were intentionally left in to remind us to do it when working on caching :)

epochs_cleaned.save(
fname_out, overwrite=True, split_naming='bids',
split_size=cfg._epochs_split_size)
# _update_for_splits(out_files, 'epochs_cleaned')
Copy link
Member

Choose a reason for hiding this comment

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

to cleanup

@larsoner larsoner merged commit 6c350e1 into mne-tools:main Sep 8, 2022
@larsoner larsoner deleted the split branch September 8, 2022 15:28
@larsoner larsoner added the EOSS4 label Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Internal assertion for outfiles triggered by splits
3 participants