fNIRS / snirf

SNIRF Format Specification
http://fnirs.org/resources/software/snirf/
Other
56 stars 33 forks source link

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 2 years ago

sstucker commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Good discussion on this so far. I am in favor of leaving the spec as is for backward compatibility in particular but also because some of the contributors to the original spec thought it was important in the first place. I too wonder what the use case. I think Ted Huppert has said he was already storing his datasets that way.

How do people feel about putting text in the appropriate place around the /nirs{i} blocks description that using this feature of SNIRF is not presently compatible with BIDS and that for those who want to remain compatible with BIDS, we encourage them to use only one /nirs block per file.

Also, seems easy enough for someone in the future to write code to break a file with multiple /nirs blocks into multiple files with single /nirs blocks each.

From: Robert Luke @.> Date: Friday, October 1, 2021 at 1:09 AM To: fNIRS/snirf @.> Cc: Boas, David @.>, Mention @.> Subject: Re: [fNIRS/snirf] Consider removing support for multiple /nirs{i} blocks: for many applications, a SNIRF file should only contain one acquisition from a single subject (#88)

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.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/fNIRS/snirf/issues/88#issuecomment-931902885, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AHFCP5EKDU6I6BKBOUF7DELUEU66TANCNFSM5FASPX5Q. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

fangq commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

I agree that no changes are needed. But as Rob says, I am curious about the ways in which people are using multiple runs per file. I would like to hear that as well.

Sent from my iPhone

On Oct 21, 2021, at 6:30 PM, Robert Luke @.***> wrote:



I'd still be keen to hear an answer to the original question from @sstuckerhttps://github.com/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.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/fNIRS/snirf/issues/88#issuecomment-949050437, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AHFCP5FWDRBAPXGR2FKV23LUICIA7ANCNFSM5FASPX5Q. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

Horschig commented 2 years ago

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 commented 3 months ago

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.