search

mne-tools / mne-bids

MNE-BIDS is a Python package that allows you to read and write BIDS-compatible datasets with the help of MNE-Python.
https://mne.tools/mne-bids/
BSD 3-Clause "New" or "Revised" License
126 stars 84 forks source link

MISC maintenance fixes #1251

Closed sappelhoff closed 1 month ago

sappelhoff commented 2 months ago

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:

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

hoechenberger commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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

hoechenberger commented 2 months ago

@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