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.

Sign up for GitHub

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

Jump to bottom

Consider removing support for multiple /nirs{i} blocks: for many applications, a SNIRF file should only contain one acquisition from a single subject #88

Open
sstucker opened this issue Sep 29, 2021 · 11 comments
Open

Consider removing support for multiple /nirs{i} blocks: for many applications, a SNIRF file should only contain one acquisition from a single subject #88

sstucker opened this issue Sep 29, 2021 · 11 comments
Assignees
@rob-luke
Labels
wontfix This will not be worked on

Comments

@sstucker
Copy link
Collaborator

We (@dboas, @rob-luke) are making an effort to make SNIRF agree with the BIDS dataset.

The BIDS specification is dependent on acquisition files belonging to a single subject, session and "run"-- with multiple runs being broken up into several files.

This means that SNIRF files that include multiple /nirs{i} groups are not BIDS compliant.

It was suggested that @fangq saves files with multiple /nirs{i} groups?

We don't want to break backwards compatibility, but wanted to discuss here the motivation for supporting /nirs{i} blocks and encouraging their use in the future.
@MichaelUM
Copy link

We only support reading multiple /nirs{i} because it was in the format but we only write a single group in Satori and never intended to add the option for multiple /nirs{i} groups because of the incompatibility with a future BIDS specification.

@fangq
Copy link
Member

fangq commented Sep 30, 2021

I feel that this is rather an issue of preprocessing, something that should be taken care of by tools like fmriprep - or equivalent for fnirs. Big feature changes like this should be ideally done before we froze v1.

also, the entire SNIRF spec does not have to be "compliant" with BIDS, as long as it can produce a file that meets BIDS requirements. Otherwise, we will have to co-develop SNIRF with BIDS, that will be a lot of constraints down the path.

@rob-luke
Copy link
Member

rob-luke commented Oct 1, 2021

This means that SNIRF files that include multiple /nirs{i} groups are not BIDS compliant.

True. Not all SNIRF files are BIDS compliant.

also, the entire SNIRF spec does not have to be "compliant" with BIDS

Again, true. Another example is TD and FD NIRS, which SNIRF support but BIDS does not.

SNIRF provides wide support for a large range of the fNIRS community including teams building their own systems etc. BIDS only supports a subset of SNIRF features and only aims to target the most prevalent fNIRS use cases by commercial devices (aims to capture 80% of the user market).

Otherwise, we will have to co-develop SNIRF with BIDS, that will be a lot of constraints down the path.

Agreed, SNIRF and BIDS do not need to be co-developed. But the BIDS development has greatly benefited from the close communication with SNIRF developers, and we appreciate the SNIRF team sharing their experiences. So keeping in regular contact benefits everyone.

We don't want to break backwards compatibility, but wanted to discuss here the motivation for supporting /nirs{i} blocks and encouraging their use in the future.

I am curious about this too. What is the use case for multiple nirs blocks. Do people use it for multiple data collection on the same participant? Or do you use it to store entire studies together? What are the pros and cons, and do you suggest it for others? I am always trying to improve my workflow.

@dboas
Copy link
Collaborator

dboas commented Oct 1, 2021 via email

@fangq
Copy link
Member

fangq commented Oct 21, 2021 •
edited
Loading

maybe we should find some time to chat about this during fNIRS2021.

I still strongly feel that there is nothing wrong for the SNIRF file used in BIDS to use only a subset of SNIRF features (which is still SNIRF compatible - meaning that SNIRF pasers should work out of box).

from Luca's talk, I think the BIDS-fNIRS developers need to think about how to deal with data duplication - in general, duplication of codes and data is not desired in programming and data management practices, because it creates extra overhead to sync between storage sites for consistency, which is a major burden down the path. If I understood it correctly, BIDS-fNIRS files extract some of the SNIRF subfields, largely probe and maybe a small part of metaDataTags to external files. I assume that if both information sources present, the external files supersede the SNIRF fields (probe is a required field in SNIRF)?

my recommendation is to find a way to reduce such duplication. I also want to understand more about the benefits of duplicating data to external files (given that a .snirf parser is already needed).

one possibility is to perhaps add a file/URI referencing syntax in SNIRF/hdf5 side to permit off-loading subfield data to separate files, but ideally, the linked file should still be a .snirf/.h5 file to be consistent with the spec. If it involves referencing .tsv/.csv/.json files, it makes parsers's workflow more difficult; linking external files via URI also makes the data parsing less robust.

if BIDS-fNIRS has had supported JSNIRF (the json wrapper of SNIRF), we would have had more flexibility, such as using JSON-LD syntax to link multiple files to remove duplication, or making data searchable without needing to externalize the subfields to text files.

@rob-luke
Copy link
Member

I still strongly feel that there is nothing wrong for the SNIRF file used in BIDS to use only a subset of SNIRF features (which is still SNIRF compatible - meaning that SNIRF pasers should work out of box).

This is the case already. I don't see what your problem or concern is.

I also want to understand more about the benefits of duplicating data to external files

Findability

one possibility is to perhaps add a file/URI referencing syntax in SNIRF/hdf5 side to permit off-loading subfield data to separate files

This is not required. But thanks for offering

@fangq
Copy link
Member

fangq commented Oct 21, 2021 •
edited
Loading

I still strongly feel that there is nothing wrong for the SNIRF file used in BIDS to use only a subset of SNIRF features (which is still SNIRF compatible - meaning that SNIRF pasers should work out of box).

This is the case already. I don't see what your problem or concern is.

oh, it was just a comment following the title of this Issue (since Luca mentioned it in his talk) - that I concur that changing SNIRF upstream spec to match BIDS restrictions is not really necessary.

if everyone agrees, I will mark it as wontfix. can still keep it open for further discussions.

@rob-luke
Copy link
Member

I'd still be keen to hear an answer to the original question from @sstucker "but wanted to discuss here the motivation for supporting /nirs{i} blocks". Do you use these and in what situations are they useful and when not? I'm always keen to learn from other peoples workflows and processes.

@dboas
Copy link
Collaborator

dboas commented Oct 22, 2021 via email

@sstucker sstucker added the wontfix This will not be worked on label Oct 31, 2021
@Horschig
Copy link
Collaborator

Horschig commented Aug 17, 2022 •
edited
Loading

We had a recent mail conversation with @dboas and @sstucker about this and a related issue (multiple devices on one subject). For us it is important that whatever we export our data to is not only compatible with the current specs, but more importantly can be used by our customers. As long as Homer3 and other widely used toolboxes or applications that can read in snirf datafiles, are not making use of it, we will not use any part of the specs that those tools do not use. For example, although snirf does allow for having multiple nirs{i} and data{j} blocks, since Homer3 cannot cope with this, we will not have an export using this part of the spec. Customers otherwise would get something from us that "doesn't work".

Having said that, I think any discussion on this point is void unless it is simultaneously being implemented in the major toolboxes. Else it stays a theoretical possibility that is impossible to use. This is no harsh critic whatsoever, I totally understand (and appreciate) the effort it takes to work on a standard dataformat and/or academic toolbox. It is just a fact that people cannot use a feature that is not supported.

Therefore, in my view, it makes sense to either disallow multiple nirs{i} blocks in snirf altogether or make Homer3 (and other toolboxes) compatible with it.

@samuelpowell
Copy link
Collaborator

Following discussion in May 24 meeting, it was decided that wording should be added to the specification to encourage the use of a single block for reasons of compatability, but that the ability to use multiple blocks should not be removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rob-luke rob-luke

Labels
wontfix This will not be worked on
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@fangq @rob-luke @Horschig @samuelpowell @MichaelUM @sstucker @dboas