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

MISC maintenance fixes #1251

Merged
merged 7 commits into from
May 28, 2024
Merged

MISC maintenance fixes #1251

merged 7 commits into from
May 28, 2024

Conversation

sappelhoff
Copy link
Member

@sappelhoff sappelhoff commented May 19, 2024

PR Description

Trying to fix #1250 ... currently no success locally, and this only seems to be a problem on macos. cc @hoechenberger

Issuing this PR to have one location where we see CI fail independent from the nightly checks.

Merge checklist

Maintainer, please confirm the following before merging.
If applicable:

  • All comments are resolved
  • This is not your own PR
  • All CIs are happy
  • PR title starts with [MRG]
  • whats_new.rst is updated
  • New contributors have been added to CITATION.cff
  • PR description includes phrase "closes <#issue-number>"

Copy link

codecov bot commented May 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.45%. Comparing base (87eea28) to head (7068f79).
Report is 24 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1251      +/-   ##
==========================================
- Coverage   97.61%   97.45%   -0.17%     
==========================================
  Files          40       40              
  Lines        8685     8685              
==========================================
- Hits         8478     8464      -14     
- Misses        207      221      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hoechenberger
Copy link
Member

@sappelhoff I can perhaps take a look later today

I'm still not convinced the inspect module is too useful these days anymore ... especially since it doesn't support the Qt browser :(

@sappelhoff
Copy link
Member Author

I'm still not convinced the inspect module is too useful these days anymore ... especially since it doesn't support the Qt browser :(

I also think that the lack of support for the newish (and amazing) Qt browser is a drawback ... and I am also not sure whether we have a ton of users who like this mne-bids feature. And I also understand how that's a bit upsetting, as you invested quite a bit of thought and work into it.

I don't know whether this is where you were going with it, but we could think about cutting that part out of mne-bids, instead of continuing to maintain it. After all, it does say that the functionality is experimental:

This functionality is still experimental and will be extended in the future. Its API will likely change. Planned features include automated bad channel detection and visualization of MRI images.

source: https://mne.tools/mne-bids/dev/generated/mne_bids.inspect_dataset.html

I personally would always use this workflow:

  1. automatically mark bad channels and segments (e.g., via pyprep and other pipelines)
  2. open the data in the QT browser, manually selecting or deselecting data features (e.g., if an automatically marked segment looks like a false positive, or if the automatic pipelines missed something)
  3. save bad channels and data segments

For this workflow, the mark_bad_channels function would arguably be enough (bad channels are marked in channels.tsv). The "bad segments" could potentially be saved in the events.tsv files, but personally, I have been saving them in txt files under a derivatives folder (sub-optimal, I know).

--> i.e., for this workflow, the interactive inspection from within mne-bids is not needed, but perhaps rather a mark_bad_segments that edits an events.tsv file with what can be parsed from mne annotations.

Speaking of mark_bad_channels, I think there are some errors in the tutorial, do you agree? Look at this part:

https://mne.tools/mne-bids/dev/auto_examples/mark_bad_channels.html#non-interactive-programmatic-bad-channel-selection

and in specific these sections:

  1. "If you instead would like to replace the collection of bad channels entirely, pass the argument overwrite=True:"

--> there is no overwrite argument for mark_bad_channels

  1. "Lastly, if you’re looking for a way to mark all channels as good, simply pass an empty list as ch_names, combined with overwrite=True:"

--> the printed results show that rather than all channels being marked as good, they are marked as bad

@sappelhoff
Copy link
Member Author

@larsoner unfortunately your suggestion does not seem to do the trick, but thanks for having a look!

@hoechenberger
Copy link
Member

@sappelhoff I'm not emotionally attached to this feature ;) I'm totally okay with removing it. I'm only worried some folks might be using it – IIRC, @adam2392 once mentioned it's part of his workflow. Not sure about @SophieHerbst

@sappelhoff sappelhoff changed the title fix inspect on macos fix inspect module tests May 21, 2024
@sappelhoff sappelhoff changed the title fix inspect module tests MISC maintenance fixes May 28, 2024
@sappelhoff sappelhoff merged commit a18d070 into mne-tools:main May 28, 2024
18 of 19 checks passed
@sappelhoff sappelhoff deleted the fixes branch May 28, 2024 08:46
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.

issue with CI and inspect module
2 participants