WIP: Add NIRS support to BIDS Validator

[rob-luke]

This PR is to add NIRS support as described in the NIRS BEP

• BEP: https://github.com/bids-standard/bids-specification/issues/438
• Example: https://github.com/bids-standard/bids-examples/pull/305

---

Note:

• [ ] Revert all links to custom branch for test data just before merging!!

[codecov[bot]]

Codecov Report

Merging #952 (dfc83d6) into master (ee7d94c) will increase coverage by 0.19%. The diff coverage is 95.12%.

@@            Coverage Diff             @@
##           master     #952      +/-   ##
==========================================
+ Coverage   83.36%   83.55%   +0.19%     
==========================================
  Files          91       91              
  Lines        3691     3758      +67     
  Branches     1123     1146      +23     
==========================================
+ Hits         3077     3140      +63     
- Misses        518      522       +4     
  Partials       96       96              

Impacted Files	Coverage Δ
bids-validator/validators/nifti/nii.js	69.49% <ø> (ø)
bids-validator/validators/tsv/checkTypeCol.js	100.00% <ø> (ø)
bids-validator/utils/summary/collectModalities.js	91.66% <50.00%> (-1.82%)	:arrow_down:
bids-validator/utils/type.js	90.25% <90.00%> (-0.01%)	:arrow_down:
bids-validator/validators/json/json.js	100.00% <100.00%> (ø)
bids-validator/validators/tsv/tsv.js	93.85% <100.00%> (+0.55%)	:arrow_up:
...ids-validator/validators/tsv/validateTsvColumns.js	52.53% <100.00%> (+0.30%)	:arrow_up:
bids-validator/validators/events/hed.js	90.90% <0.00%> (+1.07%)	:arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[rob-luke]

@sappelhoff is there a resident javascript expert I can request advice from regarding this PR? I am trying to implement the following logic from https://github.com/bids-standard/bids-specification/pull/802 but I am unable to make it work.

Logic:

• If the field ShortChannelCount is present in *_nirs.json file.
• then, the column short_channel must be present in the *_channels.tsv file.

I have been trying to replicate some of the logic from PET (https://github.com/bids-standard/bids-validator/commit/08d9a4e9994b8c512e8bb6dc973261af79f20534) but I am new to javascript and cant make sense of the logic. Any help is appreciated, even. link to similar functionality in the codebase may help put me on the right path.

[sappelhoff]

I hope @rwblair can help with that. I once had a similar issue that would involve checking JSON contingent on TSV contents (so the other way around from your issue): https://github.com/bids-standard/bids-validator/issues/673

Right now, I don't know where to start with that unfortunately.

[rwblair]

My apologies for the delay in response.

So pet_blood specifies 'requires_tsv_non_custom_columns' key/value for which tsv columns are required in its json schema spec: https://github.com/bids-standard/bids-validator/blob/master/bids-validator/validators/json/schemas/pet_blood.json#L1

Here is the location in the tsv validator where the 'requires_tsv_non_custom_columns' fields are used to make sure the proper columns exist: https://github.com/bids-standard/bids-validator/blob/c60daacf3dbb4cea5a07d5274947c366cf4c0e3a/bids-validator/validators/tsv/validateTsvColumns.js#L137

Here a specific value in a json field forces a specific TSV column to be required, but the NIRS situation is the reverse, a specific value somewhere in TSV forces a json field to become required.

I am thinking about how best to approach this and writing up some example code for the tsv validator that I'll push to this branch to see what you think.

[rob-luke]

My apologies for the delay in response.

Please don't apologise. We all have other aspects to our lives. I appreciate you taking a look at this problem. If you can push an example solution to this logic I can try and expand on that to each case where we need this behaviour. Thanks again

[rwblair]

I added a new property to the json schema 'required_if_tsv_value_present': https://github.com/bids-standard/bids-validator/blob/2b22d9884bdfca21d166abd1ec5442381b8ccd72/bids-validator/validators/json/schemas/nirs.json#L92

Specifies which column in the tsv to check against and which value should be present once. The I added additonal valdiation steps to validateTsvColumns that mirror how pet used their custom schema property. I rerused their error code it probably should be changed, BUT the evidence line there right now is probably good enough to give users an idea of whats going on:

https://github.com/bids-standard/bids-validator/blob/2b22d9884bdfca21d166abd1ec5442381b8ccd72/bids-validator/validators/tsv/validateTsvColumns.js#L201

@rob-luke how big are these tsv files typically? iteratting over the rows multiple times checking for specific values might become slow if they are massive.

I have not tested the code or written tests for it. Is there a good example dataset? Let me know what you think.

[rob-luke]

Huge thanks @rwblair.

how big are these tsv files typically?

The not very big. My hardware has 50 channels. I imagine future devices could have 2-300 channels. There would be a practical upper limit to how many sensors you could fit on a head (think EEG).

checking for specific values might become slow if they are massive.

Lets worry about that when it happens. Lets get the code working correctly first, then optimise.

I have not tested the code or written tests for it. Is there a good example dataset?

Here is a dataset that I have been testing on: https://github.com/rob-luke/BIDS-NIRS-Tapping/tree/master

I added a ShortChannelCount in the nirs.json file locally and the validator throws this error...

Is there any way I can help you debug this issue?

Let me know what you think.

This is awesome. thank you so much!

[rob-luke]

Thanks @rwblair the code now runs and ensures the variable ShortChannelCount is greater than 0. But it does not yet ensure that the required column is in *_channels.tsv. I will spend some time to add some tests etc and see what I can figure out. Thanks for your help!

[rwblair]

First batch of work committed last week or so left short_channel requirement out. The spec makes it seem as if it is always optional or recommended not required: "If the field ShortChannelCount is populated, then the optional column short_channel may be used in *_channels.tsv to describe which channels were specified as short."

ShortChannelCount is listed as recommended.

from short_channel entry: "Is the channel designated as short. The total number of channels listed as short channels should be stored in ShortChannelCount in *_fnirs.csv."

My last commit should throw an error if it ShortChannelCount is present but there is no short_channel column, if this is what we want to happen then the spec may need to be updated. I added a 'requires_tsv_non_custom_columns' property to the ShortChannelCount json schema entry specifying 'short_channel'.

For pet blood the tsv headers specified in 'requires_tsv_non_custom_columns' would only be evaluated if the json field was boolean, and set to true. I updated this logic to do this check if the field is boolean and its value is true, otherwise do the check if its 'truthy' in javascript, (not 0, false, null, etc).

Sorry I never responded with a theory of my first set of commits. Regarding testing, its much easier to debug the validator if you run it from the command-line. Notice the error from the compiled web version you posted doesn't show a filename or line number that corresponds to whats in the repository/uncompiled files. Is running the validator from the command line an option for you?

[rob-luke]

Hi @rwblair,

You are absolutely correct. I was getting confused. I was mixing up the spec and what I was wondering was possible with the validator. Thanks for sticking with me.

The spec makes it seem as if it is always optional or recommended not required: "If the field ShortChannelCount is populated, then the optional column short_channel may be used in *_channels.tsv to describe which channels were specified as short."

You are correct. My apologies. The spec is correct, I was wrong.

Sorry I never responded with a theory of my first set of commits.

No worries, to be honest I have been kinda short on time lately too.

Regarding testing, its much easier to debug the validator if you run it from the command-line. Notice the error from the compiled web version you posted doesn't show a filename or line number that corresponds to whats in the repository/uncompiled files. Is running the validator from the command line an option for you?

I haven't tried the command line usage yet. I will try that next, thanks for the tip. I am new to this type of programming, so the tips are appreciated.

As next steps I will try and add some test data and tests. And try to understand your code 😉, I will try and be self sufficient, but if I get stuck again I may ping you again. Thanks for the help

[rwblair]

So we probably want to have the short_channel thing be a warning instead of an error. As of right now there is now good way to indicate that using just the json schemas. We could imagine a new property in the json schemas "recommends_tsv_non_custom_columns" alongside "requires_tsv_non_custom_columns". It would be the same code as thevalidateJsonRequiredHeaders function just create a different Issue (Ideally we would just extend the existing function to be able to report both issues, or at least reuse the common code)

Feel free to ask as many questions as you want. I'm having trouble giving good instructions to help situate you but at least I can answer questions!

[rob-luke]

Thanks @rwblair I will get back to this soon and will surely have some more questions. I appreciate you keeping this aligned with master!

[rob-luke]

Some test data is now available at https://github.com/bids-standard/bids-examples/pull/305

[rob-luke]

Hi @rwblair and @sappelhoff, I have tried to point the tests for this PR to the bids-examples branch with fNIRS data. I can see in the test-bids-examples CI that it is indeed processing the fnirstapping data! Have I done this correctly? And is this sufficent testing? Or is there tests I need to write somewhere?

[rob-luke]

I also made a right mess of rebasing. So I hope I didn't mess anything up.

[sappelhoff]

Have I done this correctly?

looks good! But see my comment

And is this sufficent testing? Or is there tests I need to write somewhere?

I see you have already written some tests covering TSV files, you could add a couple to

https://github.com/bids-standard/bids-validator/blob/master/bids-validator/tests/json.spec.js

... following what's there for (i)MEEG

[rob-luke]

Can anyone help me to solve this problem. The tests are throwing a warning that the columns of optodes.tsv are not defined (see log here). But as they are standard column names defined in the spec I thought they did not requires a json file. Where in the validator can I disable this check on optodes.tsv? Or get it to check against the column names defined in bids-validator/bids_validator/tsv/non_custom_columns.json

[WARN] Tabular file contains custom columns not described in a data dictionary (code: 82 - CUSTOM_COLUMN_WITHOUT_DESCRIPTION)[39m
  ./sub-01/nirs/sub-01_optodes.tsv
      Evidence: Columns: name, type, x, y, z, template_x, template_y, template_z not defined, please define in: /optodes.json, /sub-01/sub-01_optodes.json,/sub-01/nirs/sub-01_optodes.json
   ./sub-02/nirs/sub-02_optodes.tsv
...

Thanks, sorry I struggle with this coding language so much

[nellh]

Can anyone help me to solve this problem. The tests are throwing a warning that the columns of optodes.tsv are not defined (see log here). But as they are standard column names defined in the spec I thought they did not requires a json file. Where in the validator can I disable this check on optodes.tsv? Or get it to check against the column names defined in bids-validator/bids_validator/tsv/non_custom_columns.json

[WARN] Tabular file contains custom columns not described in a data dictionary (code: 82 - CUSTOM_COLUMN_WITHOUT_DESCRIPTION)�[39m
  ./sub-01/nirs/sub-01_optodes.tsv
      Evidence: Columns: name, type, x, y, z, template_x, template_y, template_z not defined, please define in: /optodes.json, /sub-01/sub-01_optodes.json,/sub-01/nirs/sub-01_optodes.json
   ./sub-02/nirs/sub-02_optodes.tsv
...

Thanks, sorry I struggle with this coding language so much

Looks like optodes.tsv needed to be added to getTsvType's mapping that's used to access non_custom_columns.json. I pushed a commit with a fix for that.

https://github.com/bids-standard/bids-validator/pull/952/commits/11d2436850abc5727b027a94516763430cd85500#diff-87de614ee0e958cd1b925555c77e3b3b4ea7153847b434f8cd5c0279e309aa22L22-R23

[rob-luke]

That is amazing! Thank you so much @nellh

[rob-luke]

@rwblair and @sappelhoff I added some tests and now the coverage passes too

[sappelhoff]

@rwblair and @sappelhoff I added some tests and now the coverage passes too

looks great, thanks!

[rob-luke]

it may be beneficial to look into whether this is a validator issue or an mne-bids nirs-specific issue.

It is saying that the filename sub-01_ses-01_acq-01_optodes.tsv is not valid, which it should be. So I think its a validator issue to debug.

[rob-luke]

look into whether this is a validator issue

dd1f4f2 seems to have fixed the issue (see https://github.com/mne-tools/mne-bids/pull/1020). However, I am not very confident about my regex skills. So if anyone can take a second look over the regex that would be greatly appreciated

https://github.com/bids-standard/bids-validator/blob/dd1f4f2cb60432533716111b13cda6bbee7dd2ef/bids-validator/bids_validator/rules/file_level_rules.json#L515

[rwblair]

@rob-luke regex looks good, tested it out with a dataset with nirs with session and seemed to work. Updated the regex to match style of other rules that can match back or forward slashes for directory seperators.

[rob-luke]

Thank you kindly @rwblair !!!

[rwblair]

I believe this is ready. I'm going to let it sit for another day pending changes in the specification PR.

[sappelhoff]

@rwblair in https://github.com/mne-tools/mne-bids/pull/1061 we now have a file like this failing validation:

./sub-01/ses-01/nirs/sub-01_ses-01_acq-01_coordsystem.json

https://github.com/mne-tools/mne-bids/runs/8116368480?check_suite_focus=true#step:15:470

I believe this should pass :thinking: did the recent changes introduce an issue? cc @rob-luke

[hoechenberger]

@sappelhoff It's missing a task entity, no?

[sappelhoff]

a coordsystem shouldn't be task dependent AFAIK :thinking:

if adding a task to the file would make it valid, then something's off in the validator :thinking:

[rwblair]

@sappelhoff try with that latest commit. Silly issue in the regex that covers coordsystem suffix.