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

Add regexps for space entity: EEG and iEEG #1190

Merged
merged 9 commits into from
Feb 23, 2021

Conversation

adam2392
Copy link
Member

@adam2392 adam2392 commented Feb 15, 2021

Closes: #743
closes #1164

Adding regex pattern for iEEG space entity.

TODO

  • Add regex for MEG space not in their entities currently, although probably would be applicable (headshape + markers)?
  • Add regex for EEG
  • Update unit tests

Questions

  • How does one include the optional fields like MNI152NLin2009[a-c][Sym|Asym] (a-c and Sym/Asym) in regex?
  • Anyone have an idea where the tests are to fix for this PR?

@sappelhoff
Copy link
Member

I think tests are not in place yet for the space entity 😬

this would be a proper place: https://github.com/bids-standard/bids-validator/blob/4c1be20980b1215d45aee5e595b62c14bf2604e2/bids-validator/tests/type.spec.js#L373-L390

How does one include the optional fields like MNI152NLin2009[a-c][Sym|Asym] (a-c and Sym/Asym) in regex?

You could write it all out, like I did in the JSON schema: https://github.com/bids-standard/bids-validator/blob/4c1be20980b1215d45aee5e595b62c14bf2604e2/bids-validator/validators/json/schemas/common_definitions.json#L21-L28

Or you go over to https://regex101.com/ and check what regexp works. But I suggest the former, not the latter.

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.

please also check the session level rules, etc.

@codecov
Copy link

codecov bot commented Feb 16, 2021

Codecov Report

Merging #1190 (e5fed90) into master (4c1be20) will increase coverage by 3.43%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1190      +/-   ##
==========================================
+ Coverage   79.09%   82.53%   +3.43%     
==========================================
  Files          78       78              
  Lines        2622     2622              
  Branches      598      598              
==========================================
+ Hits         2074     2164      +90     
+ Misses        407      369      -38     
+ Partials      141       89      -52     
Impacted Files Coverage Δ
bids-validator/utils/type.js 98.94% <100.00%> (ø)
bids-validator/tests/env/FileList.js 63.33% <0.00%> (-10.01%) ⬇️
bids-validator/validators/json/json.js 100.00% <0.00%> (+1.40%) ⬆️
bids-validator/utils/bids_files.js 92.59% <0.00%> (+3.70%) ⬆️
bids-validator/validators/bids/start.js 93.33% <0.00%> (+20.00%) ⬆️
bids-validator/validators/nifti/nii.js 64.75% <0.00%> (+23.19%) ⬆️
bids-validator/validators/bids/quickTestError.js 93.33% <0.00%> (+73.33%) ⬆️

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 4c1be20...e5fed90. Read the comment docs.

@sappelhoff
Copy link
Member

@adam2392 I added some commits (including a force push for rebasing, sorry), so please before you continue to work, do:

  • git stash (in case you have any non committed changes)
  • git pull and then
  • git reset --hard origin/space
  • git stash apply (in case you had previous non committed changes that you now want to bring back)

In particular I:

@sappelhoff
Copy link
Member

@adam2392 is something holding you back from finishing this PR? Or is it more of a "finding time to do it" issue (which is absolutely fine)?

@adam2392
Copy link
Member Author

Yeah sorry didn't have time, but procrastinated a bit this morning and hopefully this is closer to being done / done. LMk.

FYI: currently the validator allows other entities into the *_electrodes.tsv and *coordsystem.json files even though they are not technically allowed in the spec.

@sappelhoff
Copy link
Member

FYI: currently the validator allows other entities into the *_electrodes.tsv and *coordsystem.json files even though they are not technically allowed in the spec.

yes, thanks for the note, see also: bids-standard/bids-examples#244

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.

I added the regexps and tests for EEG.

hopefully this is closer to being done / done

I think it's done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants