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

[MRG] fix loading without age information / single-column .tsv #912

Merged
merged 19 commits into from
Dec 20, 2021
Merged

[MRG] fix loading without age information / single-column .tsv #912

merged 19 commits into from
Dec 20, 2021

Conversation

skjerns
Copy link
Contributor

@skjerns skjerns commented Nov 16, 2021

PR Description

When loading participants.tsv with only a single column (only participant ids), p_age is empty, and can therefore not be created. See also #891 . This simple fix resolves this problem.

Merge checklist

Maintainer, please confirm the following before merging:

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

@welcome
Copy link

welcome bot commented Nov 16, 2021

Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴🏽‍♂️

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @skjerns - would you mind adding a small test here? --> https://github.com/mne-tools/mne-bids/blob/main/mne_bids/tests/test_report.py

also: it's just a small fix, but could you add an entry to the changelog? https://github.com/mne-tools/mne-bids/blob/main/doc/whats_new.rst

It'll also showcase your first PR contribution to mne-bids :-)

@sappelhoff sappelhoff added this to the 0.9 milestone Nov 16, 2021
@codecov
Copy link

codecov bot commented Nov 16, 2021

Codecov Report

Merging #912 (c69ee50) into main (8a29504) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #912   +/-   ##
=======================================
  Coverage   94.78%   94.78%           
=======================================
  Files          23       23           
  Lines        3488     3488           
=======================================
  Hits         3306     3306           
  Misses        182      182           
Impacted Files Coverage Δ
mne_bids/report.py 91.32% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a29504...c69ee50. Read the comment docs.

@hoechenberger hoechenberger modified the milestones: 0.9, 0.10 Nov 23, 2021
@hoechenberger hoechenberger marked this pull request as draft November 23, 2021 10:29
mne_bids/report.py Outdated Show resolved Hide resolved
mne_bids/tests/test_report.py Outdated Show resolved Hide resolved
mne_bids/tests/test_report.py Outdated Show resolved Hide resolved
mne_bids/tests/test_report.py Outdated Show resolved Hide resolved
mne_bids/tests/test_report.py Outdated Show resolved Hide resolved
mne_bids/tests/test_report.py Outdated Show resolved Hide resolved
doc/whats_new.rst Outdated Show resolved Hide resolved
@hoechenberger hoechenberger marked this pull request as ready for review December 18, 2021 11:21
skjerns and others added 6 commits December 18, 2021 12:46
Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>
Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>
Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>
Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>
Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>
Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>
Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

something seems to have gone wrong with rebasing and/or merging main into this PR 🤔 There are lots of changes that shouldn't be changes

@skjerns
Copy link
Contributor Author

skjerns commented Dec 18, 2021

something seems to have gone wrong with rebasing and/or merging main into this PR 🤔 There are lots of changes that shouldn't be changes

damn, I might have pulled from the wrong main. I'll tried to fix it - is it fine now?

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

looks much better now!

@sappelhoff sappelhoff dismissed their stale review December 18, 2021 16:51

alignment with main was fixed

doc/whats_new.rst Outdated Show resolved Hide resolved
@hoechenberger
Copy link
Member

Whoops, CI failing now, can you have a look? If you're busy, I might be able to take over tomorrow

doc/whats_new.rst Outdated Show resolved Hide resolved
mne_bids/tests/test_report.py Outdated Show resolved Hide resolved
doc/whats_new.rst Outdated Show resolved Hide resolved
@agramfort agramfort merged commit 5da653d into mne-tools:main Dec 20, 2021
@welcome
Copy link

welcome bot commented Dec 20, 2021

🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪

@agramfort
Copy link
Member

thx @skjerns

@sappelhoff sappelhoff mentioned this pull request Oct 22, 2024
7 tasks
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.

4 participants