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.

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

Import Snirf file #283

Merged
merged 8 commits into from
May 4, 2020
Merged

Import Snirf file #283

merged 8 commits into from
May 4, 2020

Conversation

Edouard2laire
Copy link
Contributor

Hi,
This PR makes Brainstorm compatible with a new nirs format (snirf) that is currently discussed to replace previous Homer format (.nirs) and that might be used for BIDS specification.

Snirf specification are detailed here : https://github.com/fNIRS/snirf/blob/master/snirf_specification.md

The format is based on HDF5 file format so I am using the toolbox Jsnirf (https://github.com/fNIRS/jsnirf) which is build upon eazyh5 to read it . Jsnirf can also allow us to read and write the two following format : .jsnirf and .bnirs if we add the toolboxes JSONLab and zmat. But I don't know yet if it's necessary to support them.

The PR is not ready to be merge as I still have to some part of the code for exemple to import event, or add condition on the presence of optional data. but I would like to make sure I am going in the right direction before writing too much code. I can send you exemple files if you want.

Best regards,
Edouard

@ftadel
Copy link
Member

ftadel commented Apr 24, 2020

Thanks for initiating this work!

My first question would be about the necessity of including cascades of dependent libraries to Brainstorm. Not a major problem, but may come with additional distribution and maintenance problems.
If this format is HDF5-based, maybe we could work with something lighter based on the Matlab's HDF5 support functions? See this question I posted there:
https://github.com/fangq/easyh5/issues/7

@fangq
Copy link

fangq commented Apr 24, 2020

Just to clarify - JSNIRF is basically the JSON (.jnirs) and UBJSON (.bnirs) wrapper of SNIRF (with the data serialization following the JData specification).

To read/write JSNIRF files, you need the JSONLab toolbox, which is already included in Iso2mesh. One can also use the jsonencode/jsondecode function as part if MATLAB 2018? or later to read/write .jnirs files, but you will minimally need the jdataencode/jdatadecode functions to parse the arrays in such files.

The ZMat toolbox is optional if your user is using MATLAB in the GUI mode (in such case, JSONLab uses jvm/java to compress/decompress zlib/gzip data), but to support other formats such as lzma/lzip/lz4/lz4hc and/or to use it in Octave, or matlab in the -nojvm mode, ZMat is recommended. ZMat is not included in iso2mesh.

The SNIRF format - JSNIRF is basically a JSON wrapper of SNIRF - uses HDF5 for the underlying storage. In such case, I would say use easyh5 is the simplest/easiest solution, but Octave does not support HDF5 (yet), JSON-based JSNIRF is more suitable if Octave support is needed.

@ftadel
Copy link
Member

ftadel commented Apr 27, 2020

@fangq Thanks for this detailed comments!
Note that iso2mesh is not by default redistributed with Brainstorm. Only advanced users working with FEM meshes and owning a Matlab license would download it. Two major concerns are to keep the software as compact as possible (so that people can keep on updating it monthly without much pain), and having everything available in our compiled distribution of Brainstorm (which works based on the MCR, without a Matlab license).
Many of our users actually don't have Matlab, therefore the targeted user is not exactly the same as many other Matlab-based toolboxes. Compiling external code is often a challenge technically, and complicated in terms of licensing. In the programs generated with the Matlab compiler, it is not possible to execute .m scripts that were not compiled together with the rest of the application, so no extra plugins can be downloaded at run-time. This is why we try to have everything as a solid self-contained application whenever possible, rather than a constellation of independent packages.
For each new feature, I try to evaluate if we can have an alternative compatible with this compiled distribution.

@Edouard2laire Since using the Jsnirf library would add other additional depending libraries (probably making the compilation/distribution process more complicated), I would tend to prefer a solution that relies directly on reading the SNIRF format with an HDF5-reading library (either the Matlab function, or with easyh5 if it can be compiled/redistributed with Brainstorm). I could help with this.
Do you see this being possible?

Otherwise, I'd be happy to use Jsnirf if it saves a lot of development time, but ideally it would need to meet the following criteria:

  • completely redistributable within Brainstorm together with all its dependencies
  • easy to compile Jsnirf+dependencies with the Matlab compiler
  • compatible with older Matlab versions (down to 2009b)
  • less than 1Mb total added to the Brainstorm package (scripts + compiled)

@Edouard2laire
Copy link
Contributor Author

Hi.
I tried to work on snirf format for the first time a few month ago, but I have to admit that I have not been able to make any progress using the Matlab functions as the way hdf5 files works is very obscure to me. That's why I was happy when I discovered Jsnirf as it is converting the complexe structure of hdf5 files into a simple Matlab structure when using loadsnirf.

I am not sure about using easyh5 but not jsnirf as we might end up just re-coding jsnirf and we will still have the inconvenience of using easyh5 that you pointed out before.

Maybe what I can do is finish the code using Jsnirf toolbox (so reading data from the structure created by loadsnirf) so we have the main logic of the code, and then we can look to replace this function with Matlab one (or easyh5)

@ftadel
Copy link
Member

ftadel commented Apr 27, 2020

Maybe what I can do is finish the code using Jsnirf toolbox (so reading data from the structure created by loadsnirf) so we have the main logic of the code, and then we can look to replace this function with Matlab one (or easyh5)

Works for me.
Make everything work this way, let me know when your PR is ready to be reviewed, and I'll check if there is any major issue with the compilation or redistribution. If there is, I'll try to find workarounds, but @fangq's comments are encouraging, it should not be too difficult.

Thanks for your contributions!

@ftadel
Copy link
Member

ftadel commented Apr 28, 2020

Additional discussions about EasyH5, JSONLab, OpenJData:
#284
https://github.com/fangq/easyh5/issues/7

@Edouard2laire Edouard2laire marked this pull request as ready for review April 28, 2020 14:39
@Edouard2laire
Copy link
Contributor Author

Edouard2laire commented Apr 28, 2020

Hi @ftadel

Code is ready to be reviewed. I join you the snirf version of the exemple presented in the nirs tutorial (https://neuroimage.usc.edu/brainstorm/Tutorials/NIRSFingerTapping) : https://filesender.renater.fr/?s=download&token=45fc881f-46eb-4272-82d1-2fabbbf2443e

You can also find exemple of snirf file here : https://github.com/fNIRS/snirf-samples (with a consideration to the issue I raised fNIRS/snirf-samples#5)

I might upgrade the code later to be able to handle processed data such as dOD or HbO, Hbr and HbT (see fNIRS/snirf#40)

I also have some question regarding the importation :

  • each auxiliary channel in snirf have his own time course. What shall we do if this time course is not compatible with the main nirs one ?

  • Can you confirm me that all coordinates in brainstorm are in meter ? I think I saw it written somewhere but I am not able to find this information again.

@ftadel
Copy link
Member

ftadel commented Apr 29, 2020

each auxiliary channel in snirf have his own time course. What shall we do if this time course is not compatible with the main nirs one ?

It's not possible to save in one file signals with different time vectors (different recording period or different sampling rate). If you want to be able to keep them together with the NIRS recordings, you'd need to re-interpolate on the NIRS time vector with the interp1 function.

Can you confirm me that all coordinates in brainstorm are in meter ?

Yes, everything is in international units.
Coordinate systems are described here: https://neuroimage.usc.edu/brainstorm/CoordinateSystems

@fangq
Copy link

fangq commented Apr 30, 2020

@ftadel, please see a summary of the licenses/disk budget for the dependencies involved

completely redistributable within Brainstorm together with all its dependencies

All dependencies are released under GPL v3 compatible licenses:

  • jsonlab: GPLv3 and BSD-3 dual license, MATLAB/Octave
  • zmat: GPLv3, MATLAB/Octave
  • easyh5: GPLv3 and BSD-3 dual license, MATLAB (Octave does not have HDF5 support)
  • jsnirfy: GPLv3 and Apache-2 dual license, MATLAB/Octave (for JSNIRF files)

also, not related to this issue, but #284

  • jnifti: GPLv3 and Apache-2 dual license, MATLAB/Octave

among these, jsonlab/jsnirfy/jnifti are already included in iso2mesh.

easy to compile Jsnirf+dependencies with the Matlab compiler

I've successfully compiled most of the iso2mesh-included units in the i2m GUI and they compiles without any problem.

https://github.com/fangq/iso2mesh/blob/master/i2m.prj#L95-L181

compatible with older Matlab versions (down to 2009b)

These toolboxes should be compatible to at least R2010a because that's the version I am using for my development. I also test those on Octave 4.2 and later versions of MATLAB. Happy to install 2009b and test, but I don't expect any major problem.

less than 1Mb total added to the Brainstorm package (scripts + compiled)

below, I summarized the dependency package sizes before and after compression.

Component License Uncompressed Size Zipped Min Size(Zipped)
jsonlab GPLv3/BSD3 250kB 85kB 54kB
zmat GPLv3 140kB-1.1MB mex 77KB-300kB (upx -9) 77kB-mexw64, 300KB-mexa64
easyh5 GPLv3/BSD3 99kB 35kB 35kB
jsnirfy GPLv3/Apache2 72kB 28kB 28kB
jnifti GPLv3/Apache2 108kB 37kB 37kB

so, to add jsonlab, easyh5, jsnirfy and jnifti to brainstorm only cost 187kB of compressed space (all native matlab/octave code, including documentations).

zmat is a mex file based package, which contains 3 pre-compiled mex files for windows/mac/linux. After exe-packing using upx -9 (which does not impact utility), the min file size is 77kB to 300kB per platform, or ~500kB combined. Again, If brainstorm already supports stream-level compression, this can be replaced. Also, I can remove the LZMA/LZip support (-DNO_LZMA), that drops the package size by 50% or more.

also, a lot of the functions are "wrappers" for easy utility, and can be stripped if more space is needed.

let me know if you have further questions.

@ftadel
Copy link
Member

ftadel commented May 1, 2020

Thanks @fangq, this sounds perfect!
I'll go over the integration of @Edouard2laire's code next week, and will let you know how it goes.

As you sound totally fine with it, I will copy the libraries I need (at least easyh5) directly to our external code folder (https://github.com/brainstorm-tools/brainstorm3/tree/master/external).

@ftadel ftadel merged commit 6911186 into brainstorm-tools:master May 4, 2020
@ftadel
Copy link
Member

ftadel commented May 4, 2020

@Edouard2laire Have you forgotten to push out_fwrite_snirf.m?

@Edouard2laire
Copy link
Contributor Author

Hi. Thanks a lot

@Edouard2laire Have you forgotten to push out_fwrite_snirf.m?
Oups, actually I didn't wrote that function and it didn't got called during my test. What is the difference between export_data that call the out_snirf function I pushed and out_fwrite that call out_fwrite_snirf ?

@ftadel
Copy link
Member

ftadel commented May 4, 2020

What is the difference between export_data that call the out_snirf function I pushed and out_fwrite that call out_fwrite_snirf ?

The functions out_fopen/out_fwrite are for saving continuous binary files in multiple calls.
export_data is for file types that can be fully exported at once.

I am reformatting everything so that it fits all the Brainstorm conventions and looks similar to all the other existing reading functions. I apologize in advance if you don't recognize much of your initial code at the end. Will post it soon.

@Edouard2laire
Copy link
Contributor Author

Edouard2laire commented May 4, 2020

oh ok. Thank you. Sorry to cause you extra-work.

The functions out_fopen/out_fwrite are for saving continuous binary files in multiple calls.
export_data is for file types that can be fully exported at once.

so, I guess it the same in in_fopen/in_fread ? A strange thing I notice, is in_fopen_nirs_brs, the code doesn't seems to load the data but only load the nirs montage. (but I guess then brainstorm call in_fread_nirs_bsr to read data)

@ftadel
Copy link
Member

ftadel commented May 4, 2020

A strange thing I notice, is in_fopen_nirs_brs, the code doesn't seems to load the data but only load the nirs montage. (but I guess then brainstorm call in_fread_nirs_bsr to read data)

Indeed, the in_fopen and out_fopen functions only read/write the header, no data.
I've renamed your in_fopen_snirf into in_data_snirf, as it reads a full structure instead of only a header.

@ftadel
Copy link
Member

ftadel commented May 4, 2020

I pushed the modified reader in this commit (includes easyh5 and jsnirfy) :
d236104

@fangq Easyh5 doesn't work with 2009b or 2010a, for reading @Edouard2laire's example.snirf (https://filesender.renater.fr/?s=download&token=45fc881f-46eb-4272-82d1-2fabbbf2443e)

** Error: Line 40: Input argument "plist_id" is undefined.
** 
** Call stack:
** >open.m at 40
** >loadh5.m at 63
** >loadsnirf.m at 39

With 2015b, I get this warning:

Warning: The function 'H5ML.hdf5lib2' returned an mxArray with non-temporary scope. 
> In H5A.iterate (line 75)
  In loadh5>group_iterate (line 177)
  In H5L.iterate (line 63)
  In loadh5>load_one (line 123)
  In loadh5>group_iterate (line 155)
  In H5L.iterate (line 63)
  In loadh5>load_one (line 123)
  In loadh5>group_iterate (line 155)
  In H5L.iterate (line 63)
  In loadh5>load_one (line 126)
  In loadh5 (line 90)
  In loadsnirf (line 39)

With 2014b: jnirs.nirs.data.measurementList is a cell array while it is an array of struct other versions...

With 2014b: Your neuro_run01.snirf does this:

** Error: Line 63:  hdf5lib2
** Link iteration failed.   hdf5lib2
** Link iteration failed.   hdf5lib2
** Link iteration failed.   hdf5lib2
** Link iteration failed.  Undefined function or variable "cdata_out".
** .
** .
** .
** .
** 
** Call stack:
** >iterate.m at 63
** >loadh5.m>load_one at 126
** >loadh5.m at 90
** >loadsnirf.m at 39

I didn't test other Matlab versions, but it looks like some more exhaustive testing across Matlab versions should be done.

@fangq
Copy link

fangq commented May 4, 2020

sorry, I meant that JSON-based JSNIRF files should have a broader compatibility (JSONLab were tested on R2010a), but the HDF5 based SNIRF file were developed mostly on R2016, and I did not run sufficient test on other versions. I will do that this week.

It appears that MATLAB's HDF5 APIs were added in R2011a, so, you were right, R2009-R2010 does not support this toolbox. I just ran this on R2013a and eazyh5 only works if I do not have empty arrays. I am going to do more tests and improve the code for better compatibility. Will create a ticket in easyh5 to follow up.

@Edouard2laire Edouard2laire deleted the snirf branch May 4, 2020 19:13
@fangq
Copy link

fangq commented May 5, 2020

this is partially fixed (fangq/easyh5@600c2d1) - for matlab older than R2011, it exits and gives an error that hdf5 is not supported. For matlab 2011-2014, its HDF5 support is quite buggy, so I let loadh5 to read datsets in alphabetic order by default (instead of the preferred creation-order). The errors went away.

It works without any issue on R2016 and 2018, but I haven't installed R2015 to find out what has caused the warnings. will continue updating it.

@ftadel
Copy link
Member

ftadel commented May 5, 2020

Reported to Brainstorm here: ff6e180

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants