search

fNIRS / snirf

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

ICNNA v1.2.0 supports .snirf #135

Closed forihuelaespina closed 9 months ago

forihuelaespina commented 1 year ago

ICNNA v1.2.0 now supports .snirf (long due!). Testing thus far is limited.

Current limitations

Additionally, there are some inherent differences that may require attention; 1) Meta-data ICNNA stores subject and session information in objects different from the @nirs_neuroimage. Therefore, while writing exporting to .snirf file, the snirf meta-data related to this cannot be extracted from the nirs_neuroimage. Only "generic" default meta-data is created. Users can of course later change this by manipulating the object's attributes before saving to a file.

2) Wavelengths in the snirf probe ICNNA stores wavelength information in the @rawData objects different from the @nirs_neuroimage (the rawData is accessible via the @dataSource object containing the @nirs_neuroimage when the @dataSource is locked). Therefore, the snirf data related to this cannot be extracted from the @nirs_neuroimage. Users can of course later change this by manipulating this object's attribute before saving to a file.

3) Stims additional data and dataLabels Both snirf and ICNNA can store additional information for each event in the timeline. However, in snirf, this additional information are simply additional columns in the stim(i).data matrix, whereas ICNNA associates a cell array to the condition and hence can store more rich data than just scalar values associated to the events. In this sense, not all data in ICNNA can be exported to snirf. To avoid potential conflicts, by the time being, ICNNA's eventsInfo is not being exported to the final snirf object (not even the scalar information if any). If a user wants to preserve this, this ought to be done manually after the conversion.

4) Aux data ICNNA stores auxiliary information collected during the sessions in different @dataSource objects. Therefore, the snirf data related to these cannot be extracted from the @nirs_neuroimage. You can of course later change this by manipulating this object's attribute before saving to a file.

Horschig commented 1 year ago

Thanks for letting us know. But I do not understand why this has to be an issue on the snirf repository? Is this here a specific request to change something or work on something?

forihuelaespina commented 1 year ago

Thanks Jörn! Certainly not a request (I do not expect snirf to adapt to other software requirements) but more intended as documentation. I wasn't sure where would be the best place to report this, so I thought here would be the less intrusive. I´m happy to close it here and move it wherever you consider is more appropriate.

Horschig commented 1 year ago

https://github.com/fNIRS/snirf/discussions/ could have been a good option. Or we could consider making a wiki page with pages for supported software and some description. I think the latter would be good in helping users find their way and also working around issues. Any thoughts on this @samuelpowell @rob-luke @dboas ?

samuelpowell commented 1 year ago

There is a list of software package which support the SNIRF format in the README file of this repository. I don't see a problem with a PR that adds ICNNA (with the caveat that at some point we probably need to formally check the level of support in these packages, and prune accordingly).

rob-luke commented 1 year ago

Congratulations on the great package!

I agree, opening a PR to add this new package to the Readme is a good idea to help people find this tool.

forihuelaespina commented 1 year ago

The PR/discussion sounds great. However, I do not seem to have access to the discussions to move it there. @samuelpowell : Please note that the list in the front README does not include information about specific issues on a given implementation e.g. the ones above. It may be misleading to users, who may misinterpret the degree of support.

PS: Thanks, @rob-luke!

Horschig commented 1 year ago

However, I do not seem to have access to the discussions to move it there.

I opened the Discussion section. We can see if it's useful or not.

samuelpowell commented 1 year ago

@Horschig I'd advise against opening the Discussions section without proper consideration, because:

@forihuelaespina I agree that the list needs to be audited, but in the mean time I expect a PR would be accepted.

Horschig commented 1 year ago

@Horschig I'd advise against opening the Discussions section without proper consideration, because:

  • there is no policy for how this is managed or what goes there
  • no one has agreed to support / manage the content
  • it's tied to GitHub, not a part of the repository

@forihuelaespina I agree that the list needs to be audited, but in the mean time I expect a PR would be accepted.

feel free to close it, I don't mind

dboas commented 9 months ago

Has this issue been resolved?

forihuelaespina commented 9 months ago

Has this issue been resolved?

I still can't see the PR/discussion on my end, so I reckon it hasn't but I may be wrong.

Horschig commented 9 months ago

The discussions section is still open: https://github.com/fNIRS/snirf/discussions but no one is using it, and there is no update. So indeed, nothing has happened here. I think it is up to @forihuelaespina to make a PR or add a discussion.

Re closing, I would suggest to close stale issues after a year of no activity, otherwise they will just pile up.

dboas commented 9 months ago

@forihuelaespina I think the issues you brought up about differences between ICNNA and SNIRF should be documented in the ICNNA documentation, not on the SNIRF specification github. If you have specific suggestions on how SNIRF can or should be modified, we welcome those changes.

We will certainly accept a pull request to the ReadMe listing ICNNA as supporting SNIRF.

forihuelaespina commented 9 months ago

Thanks everyone! I have now moved the intial post about the limits of the support to ICNNA's github, and made a pull request as suggested by @dboas I reckon we can now close this issue for good.