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 doc and logging mark_channels #1293

Merged
merged 6 commits into from
Aug 19, 2024
Merged

Conversation

sappelhoff
Copy link
Member

PR Description

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>"

@sappelhoff sappelhoff requested a review from drammock August 16, 2024 09:45
@@ -2565,7 +2564,7 @@ def mark_channels(bids_path, *, ch_names, status, descriptions=None, verbose=Non
idx = tsv_data["name"].index(ch_name)
logger.info(
f"Processing channel {ch_name}:\n"
f" status: bad\n"
f" status: {status_}\n"
Copy link
Member Author

Choose a reason for hiding this comment

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

no idea why this was just bad before 🤔 especially because the column is initialized with all good if it wasn't present before.

    # Read sidecar and create required columns if they do not exist.
    if "status" not in tsv_data:
        logger.info('No "status" column found in input file. Creating.')
        tsv_data["status"] = ["good"] * len(tsv_data["name"])

I think this is also why in the docstring it said something about "default is 'bad'". If we start writing the status for a few channels only, the status for all other channels gets automatically set. However it gets set to "good", not "bad", ... which I find reasonable, but which was misleading in the function docstr.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, I followed up on this in 5c04279

@sappelhoff
Copy link
Member Author

Thanks for making me aware of this @drammock, I think the present PR should fix a thing or two.

Copy link
Member

@drammock drammock left a comment

Choose a reason for hiding this comment

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

I'm about to open a separate issue about this, but FWIW the behavior of "empty list means mark all channels" was really surprising and caused me to have to alter and re-run my dataset preparation script (no big deal, this is why we dry-run those scripts with only 2 subjs/2 sessions).

doc/whats_new.rst Outdated Show resolved Hide resolved
mne_bids/write.py Outdated Show resolved Hide resolved
mne_bids/write.py Outdated Show resolved Hide resolved
sappelhoff and others added 3 commits August 16, 2024 20:21
Co-authored-by: Daniel McCloy <[email protected]>
Co-authored-by: Daniel McCloy <[email protected]>
Co-authored-by: Daniel McCloy <[email protected]>
@sappelhoff
Copy link
Member Author

Thanks for the review, yes let's discuss your concerns in a separate issue.

mne_bids/write.py Outdated Show resolved Hide resolved
@sappelhoff sappelhoff enabled auto-merge (squash) August 19, 2024 08:53
Copy link

codecov bot commented Aug 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.45%. Comparing base (87eea28) to head (8f77101).
Report is 55 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1293      +/-   ##
==========================================
- Coverage   97.61%   97.45%   -0.17%     
==========================================
  Files          40       40              
  Lines        8685     8717      +32     
==========================================
+ Hits         8478     8495      +17     
- Misses        207      222      +15     

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

@sappelhoff
Copy link
Member Author

test failures are unrelated, merging.

@sappelhoff sappelhoff disabled auto-merge August 19, 2024 09:28
@sappelhoff sappelhoff merged commit 283d859 into mne-tools:main Aug 19, 2024
19 of 22 checks passed
@sappelhoff sappelhoff deleted the mark branch August 19, 2024 09:28
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.

docstring for mark_channels is misleading
2 participants