[ENH] BEP030: Functional Near-Infrared Spectroscopy #802

rob-luke · 2021-05-22T10:51:53Z

BEP030: Functional Near-Infrared Spectroscopy
This PR adds functional near-infrared spectroscopy to BIDS.
Leads: @rob-luke & @lpollonini

validator: Add NIRS support to BIDS Validator bids-validator#952
examples: BEP030: Add fNIRS examples to master branch bids-examples#333
rendered specification draft: https://bids-specification.readthedocs.io/en/bep030/04-modality-specific-files/11-functional-near-infrared-spectroscopy.html

Progress:

Community reviewed proposal (google docs)
Transition google doc to markdown (this PR). View current render here.
Fix language in document. It currently reads as if it was written by many people (as it was), and needs to be reworked to fix sentence structure and grammar.
Extend validator to support this BEP (WIP: GitHub PR link)
Generate open datasets from several labs and ensure they are compliant with specification (e.g. dataset from rob-luke, dataset from robertoostenveld, these are listed in the manuscript)
Add example dataset to bids-examples (dataset 1, dataset 2)
Resolve remaining issues in this PR as described here
Preprint of the new extension (manuscript hs been reviewed by half the authors, about to be circulated to remaining authors).
Press release

NOTE: At all stages in this process we will actively advertise and invite feedback on the proposal. We invite all feedback no matter how minor or major. And we will advertise the draft website to the community for review once it is rendering correctly.

closes #438

Scope of proposal:

This specification applies to the storage of raw fNIRS data and metadata, and does not address data processing or the storage of processed data. This specification aims to deal with data from well established commercial systems, not experimental or DIY systems, although everyone is encouraged to adhere to BIDS-fNIRS to facilitate future data sharing. The specification currently only addresses continuous wave (CW) fNIRS devices, but consideration is taken not to preclude extensions to TD and FD instruments in the future upon contributions from experts in these domains.

Considerations when reviewing this BEP:

It was decided to only include continuous wave (CW) fNIRS in this BEP. However, we have taken effort not to preclude expansion to other techniques such as time or frequency domain fNIRS. We ask that TD and FD experts still review this proposal to ensure nothing in the specification precludes future expansion to other fNIRS types.
This BEP does not specify details about hyper scanning, which is popular in the fNIRS community. This was decided as hyper scanning is not specific to fNIRS and is also performed using other modalities. Thus, hyper scanning should be added to BIDS in a modality agnostic manner. Hyperscanning is an open issue in BIDS (hyperscanning data storage #402).
A conscious decision was made not to specify what should be considered a short channel. The specification contains fields to store the number of short channels, as this will be important for searchability. But the definition of what exactly constitutes a short channel (is it less than 8mm, or 10mm, etc) is left to the end user and software packages.

To provide feedback:

Select Files Changed in the tab list at the top of this text box.
Press the Load Diff button for the src/04-modality-specific-files/10-functional-near-infrared-spectroscopy.md file
Hover your mouse over any line that you wish to comment on, this should make a blue cross appear to the right of the line.
Press the blue cross and provide enter your comment

rob-luke · 2021-05-31T01:48:38Z

@sappelhoff do you know why the types aren't rendering as hyperlinks in my tables? I think I copied the format from the other BEPs. Thanks

sappelhoff · 2021-05-31T07:02:40Z

@sappelhoff do you know why the types aren't rendering as hyperlinks in my tables? I think I copied the format from the other BEPs. Thanks

you probably forgot to copy over the link targets, the [this is a link][] syntax only works if you declare [this is a link]: https://www.google.com somewhere in the document (that latter declaration will be invisible in the rendering).

see: https://github.com/bids-standard/bids-specification/blame/master/src/04-modality-specific-files/03-electroencephalography.md#L433-L447

rob-luke · 2021-05-31T07:05:13Z

Thanks so much @sappelhoff, that makes total sense now. Appreciate it.

src/04-modality-specific-files/10-functional-near-infrared-spectroscopy.md

effigies

We merged some table-rendering code that will generate the tables from the schema (see 3ce06ae). There were some inconsistencies between the schema tables and the ones rendered in text, so I took the ones rendered in text as authoritative. Please review the rendered document to verify.

Also I have some questions about the ordering constraints that are in the schema tables but were not in the text. Now they'll be rendered, so please make sure that's actually what you intended:

effigies · 2022-09-30T00:48:17Z

src/schema/rules/tabular_data/nirs.yaml

+  initial_columns:
+    - name__channels
+    - type__nirs_channels
+    - source__channels
+    - detector__channels
+    - wavelength_nominal
+    - units__nirs


There was nothing in the spec text that said that the order of these columns was intended to be enforced. Is the intention that these be enforced? This section could be shortened or dropped, if the order of columns isn't important.

Side question: I assume the name column is intended to have unique values. Is that correct?

Great catch @effigies
Yes, lets enforce order of these columns. I will add this to the text of the spec.

@rob-luke The order requirement is now part of the table. No need to add more text, IMO.

Thanks @effigies

effigies · 2022-09-30T00:48:51Z

src/schema/rules/tabular_data/nirs.yaml

+  initial_columns:
+    - name__optodes
+    - type__optodes
+    - x__optodes
+    - y__optodes
+    - z__optodes


Again, is this order meant to be enforced? And is name going to be unique?

Yes, order should be enforced, I will add to spec text.
Name MUST be unique. I will add this to the spec text. Is there a way to make the validator enforce that name is unique?

Yes, we'll implement it with #1304, so again, I don't think you need to do anything. I just wanted to make sure that my interpretation was correct.

Thanks @effigies

effigies · 2022-09-30T01:18:51Z

src/schema/rules/checks/nirs.yaml

+RequiredChannels:
+  selectors:
+    - datatype == "nirs"
+    - suffix == "optodes"
+    - extension == ".tsv"
+  checks:
+    - associations.channels != null


I didn't see this rule in the spec. Is channels.tsv required if optodes.tsv is present? Or is it just that channels is required?

channels.tsv is RECOMMENDED. optodes.tsv is REQUIRED if channels.tsv is present.
I just clarified this in the text. Thanks for highlighting it.

effigies · 2022-09-30T01:25:04Z

src/schema/rules/checks/nirs.yaml

+    - intersect(columns.x, ["n/a"])
+  checks:
+    - columns.template_x != null


This is a limited check that just says that the column template_x must exist if there are any n/a in x. I can't think of a way of verifying that template_x is non-n/a every time that x is n/a with our current set of operations, but I suspect this check would catch >90% of honest mistakes.

If it's important to get the full check in the validator, we can think more about what functions need to be added to our expression language.

I suspect this check would catch >90% of honest mistakes.

I agree this should be sufficent.

rob-luke · 2022-09-30T03:58:07Z

Thanks for the code changes and review @effigies

sappelhoff

Thanks all for your work on this.

I am +1 on merging this one to avoid merge conflicts and make this PR longer and longer. Instead I propose we resolve the two remaining issues from #802 (review) in standalone PRs after this merge but before the upcoming release.

If I get a couple more maintainer approvals I'll go ahead and hit the button.

src/schema/objects/columns.yaml

rob-luke · 2022-10-03T10:23:53Z

I have made the variable name changes as requested by @tsalo #802 (review)

sappelhoff · 2022-10-05T08:23:12Z

As discussed in yesterday's maintainers meeting I am merging this PR now -- congrats to all who helped and especially to @rob-luke and @lpollonini! 🎉 🥳

We can now continue to look for bugs and work on the few remaining issues (mainly NIRS vs fNIRS and Gyro vs AngVel).

Once the remaining issues are resolved we can initiate the release protocol and go forward with BIDS 1.8.0 🚀

lpollonini · 2022-10-05T08:27:18Z

Thanks @sappelhoff !

I am revising and formatting the manuscript for submission to Scientific Data, and will distribute a cloud copy to all co-authors/contributors for their input right after the upcoming fNIRS 2022 conference.

rob-luke · 2022-10-05T08:41:15Z

WooHoo!!!!!! 🎉

Thank you to everyone who contributed. Now let's knock these last few issues off 🚀

Horschig · 2022-10-05T09:11:37Z

awesome work, the community will thank you ;)

Remi-Gau · 2022-10-05T09:37:20Z

Congrats everyone!!!

helenacockx · 2022-10-05T09:46:16Z

Hooray!!! 🥳🥳🥳

rob-luke · 2022-10-05T10:07:07Z

We can now continue to look for bugs and work on the few remaining issues (mainly NIRS vs fNIRS and Gyro vs AngVel).

Regarding these two issues:

NIRS vs fNIRS: We will discuss this on Monday at the fNIRS conference, in the SNIRF meeting at midday. Please attend or contribute your opinion in advance. A decision will be made at this time.
Gyro vs AngVel: I suggest we wait a little longer (another week?) for more contributions to the discussion at https://groups.google.com/g/bids-discussion/c/JH9t06EA7UQ/m/V5Z0z2xVBgAJ before we address this

Thanks for the merge @sappelhoff and extensive contributions from all @bids-maintenance @bids-standard/maintainers @bids-standard/steering over the last year(s)

Remi-Gau · 2022-10-05T10:53:32Z

Starting testing things on NIRS things with bids-matlab.

Should nirs be mentioned in the modalities of the schema: https://github.com/bids-standard/bids-specification/blob/master/src/schema/objects/modalities.yaml

If I get a 👍🏾 I will open a separate issue and PR to fix (ideally before release)

sappelhoff · 2022-10-05T15:26:25Z

NIRS vs fNIRS: We will discuss this on Monday at the fNIRS conference, in the SNIRF meeting at midday. Please attend or contribute your opinion in advance. A decision will be made at this time.

one thing to consider is the modality folder names (like "beh" for behavioral, "anat" for anatomical MRI data, etc.) --> if it's fnirs, it'll be longer than nirs. Maybe not a strong argument, but one to keep in mind.

rob-luke · 2022-10-06T00:00:08Z

@sappelhoff I think I might be misunderstanding what we are discussing, so let me overcommunicate so we can ensure I am trying to solve the correct problem.

The naming of the modality is currently nirs in the schema and validator. So a file in a BIDS dataset might have the path /sub-21/nirs/sub-21_task-fingerautodual_nirs.snirf and /sub-21/nirs/sub-21_task-fingerauto_nirs.json. Further, the field names include the prefix NIRS, for example NIRSCoordinateSystem.

Currently, the text in the specification (as rendered on ReadTheDocs) has the title functional Near-Infrared Spectroscopy and uses the term fNIRS frequently (30 times) in the document.

We are not debating if the schema and file names should be changed from nirs.
We are discussing if the docs page should be reworded to consistently use NIRS, rather than fNIRS (including dropping functional from the title and other places in the text).

Is my above understanding of the problem statement correct? I think the current mixing of fNIRS and NIRS is ok, but I will also be fine with changing everything in the written document from fNIRS to NIRS if thats what the community prefers. I do not think we should change the schema and validator to usefnirs, its longer and not consistent with other modalities.

sappelhoff · 2022-10-06T05:57:11Z

Okay, thank you for clarifying, Rob. I am fine with your outline of the problem.

sappelhoff · 2022-10-12T18:14:39Z

There seems to be a consensus for the AngVel vs Gyro discussion now: https://groups.google.com/g/bids-discussion/c/JH9t06EA7UQ
did you finish discussing nirs vs fnirs?

Given that these are the last open items before a release, it'd be great to wrap this up :-)

lpollonini · 2022-10-16T09:02:03Z

There seems to be a consensus for the AngVel vs Gyro discussion now: https://groups.google.com/g/bids-discussion/c/JH9t06EA7UQ

I think GYRO it is

did you finish discussing nirs vs fnirs?

I am not sure if the discussion has concluded, but I am also OK with converting fNIRS to NIRS in the docs, if the mixing with the schema/validator could generate confusion. Also the SNIRF data format does not contain the word "functional", and that's one more reason to drop it from the doc in consistency with everything else.
If we proceed with NIRS only, we should start off the doc with something like "In this doc, the term NIRS refer mostly, if not uniquely, to functional NIRS [...]" to clarify what we are talking about.

rob-luke · 2022-10-16T17:48:25Z

I am also OK with converting fNIRS to NIRS in the docs

Agreed, I will make the changes.

rob-luke · 2022-10-17T13:08:22Z

@bids-standard/maintainers

Given that these are the last open items before a release, it'd be great to wrap this up :-)

These are done. Please let me know if there is anything else, but I think we are good to wrap this up!!

rob-luke added the BEP label May 22, 2021

rob-luke force-pushed the bep030 branch 6 times, most recently from d940087 to 1ebcd72 Compare May 29, 2021 01:48

rob-luke mentioned this pull request May 31, 2021

Add NIRS support to BIDS Validator bids-standard/bids-validator#952

Merged

1 task