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

[WIP] Participants data always null. Issue 49 never thrown. #1772

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rwblair
Copy link
Member

@rwblair rwblair commented Aug 8, 2023

Participants was being defined as a const and assigned null. Usual use case for consts is to give it an actual array or object with reference and then later on push to that array or object. For whatever reason tsv validation attempts to reassign the participants reference it gets.

…ets reassigned, declare with let not const""

This reverts commit 5ec0b3d.
@codecov
Copy link

codecov bot commented Aug 8, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -2.32% ⚠️

Comparison is base (5ec0b3d) 85.71% compared to head (8122bcb) 83.40%.

❗ Current head 8122bcb differs from pull request most recent head cf46045. Consider uploading reports for the commit cf46045 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1772      +/-   ##
==========================================
- Coverage   85.71%   83.40%   -2.32%     
==========================================
  Files         130       92      -38     
  Lines        6575     3893    -2682     
  Branches     1524     1270     -254     
==========================================
- Hits         5636     3247    -2389     
+ Misses        833      548     -285     
+ Partials      106       98       -8     
Files Changed Coverage Δ
bids-validator/validators/bids/fullTest.js 98.92% <100.00%> (ø)

... and 38 files with indirect coverage changes

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

@rwblair rwblair changed the title [WIP] Change participants variable decleration to let. [WIP] Participants data always null. Issue 49 never thrown. Aug 8, 2023
…t tests in case there is no participants file
@rwblair
Copy link
Member Author

rwblair commented Aug 8, 2023

We now get code 67 NO_VALID_DATA_FOUND_FOR_SUBJECT for any bids-examples with a participants.tsv.

ieeg_motorMiller2007 has rows in participants.tsv that have no directories in the dataset. Throws code 49 PARTICIPANT_ID_MISMATCH

@sappelhoff
Copy link
Member

ieeg_motorMiller2007 has rows in participants.tsv that have no directories in the dataset. Throws code 49 PARTICIPANT_ID_MISMATCH

@dorahermes could you look into this, please?

@rwblair
Copy link
Member Author

rwblair commented Aug 10, 2023

Suggestion from maintainers meeting: for legacy validator automatically add 67 to ignored codes if --ignore-nifti-headers used, prevent us from having to edit ignored codes in bids-examples tests.

@dorahermes
Copy link
Member

It seems that 3 subjects are missing folders (jf, gf, jp). Data from these subjects is located in the original ECoG library. Let me know if you prefer option 1 or 2:

  1. Remove these three subjects from the BIDS example participants.tsv file
  2. I could convert these 3 subjects to BIDS and do a pull request to update accordingly

(I am not sure why I missed these subjects in the conversion. One thing I vaguely remember is that there was a mismatch in electrodes and channels, but the current version of the ECoG library does not have any mismatches. There was an update in 2022 though.)

@effigies
Copy link
Collaborator

IMO if the usefulness of the dataset as an example isn't damaged by dropping the subjects, removing them from participants.tsv seems sensible to me.

@sappelhoff
Copy link
Member

IMO if the usefulness of the dataset as an example isn't damaged by dropping the subjects, removing them from participants.tsv seems sensible to me.

agreed

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.

Participant data not being returned
4 participants