Integration of the SNIRF reader in Brainstorm

ftadel commented 4 years ago

@Edouard2laire @thomas-vincent

Some functions from nirstorm are needed to use the new SNIRF reader in Brainstorm: https://github.com/brainstorm-tools/brainstorm3/tree/master/external/nirstorm

One needs a modification (additional returned parameter): https://github.com/brainstorm-tools/brainstorm3/blob/master/external/nirstorm/nst_format_channel.m

I'm not sure how you want to deal with this in nirstorm: removing them from the nirstorm repo/package?

Edouard2laire commented 4 years ago

Hi Francois,

Thanks for that. yes I think we will delete them after PR #123 is merged.

Can you confirm me that calling nst_format_channel with only one output (eg channel_label= nst_format_channel(isrc, idet, measure) ) will still work or we need to make change in the code to "discard" the second output ?

edit: I just made the test to be sure, and indeed it's working so we won't have side effect in nirstorm.

ftadel commented 4 years ago

Can you confirm me that calling nst_format_channel with only one output (eg channel_label= nst_format_channel(isrc, idet, measure) ) will still work or we need to make change in the code to "discard" the second output ?

The modified function from Brainstorm would work with Nirstorm (it never hurts to add returned parameters). But having Nirstorm+Brainstorm both in the path may cause the SNIRF reader in Brainstorm to stop working (as it doesn't have the missing returned parameter)

thomas-vincent commented 4 years ago

It is better to isolate everything needed to load snirf in brainstorm and not depend on nirstorm. There may be some issues if they stay there since the same functions in nirstom will shade them.

If you really need to share the functions, then the functions in question should be moved in brainstorm. But as I can see, it is very basic short functions, I think you can fork their content. Everything in brainstorm/external should be removed and integrated where needed in brainstorm io.

Edouard2laire commented 4 years ago

I don't have a clear opinion on that subject but I see 3 options here :

First, as suggested by Thomas, we remove the function from the brainstorm external folder and put them and the files where they are called so (out_data_snirf.m and in_data_snirf.m). This solution leads to have 3 copy of the functions.
Second, as suggested by Francois, we remove the function from NIRSTORM repository and keep them in brainstorm. That lead to only one copy of the function but make that the NISTORM code is spread between two repository which can cause trouble is guess (like if someone update nirstorm but use an old version of brainstorm)
Third, a compromise solution, we leave the two versions as is but keep them synchronized. So here we change nst_format_channel accordingly to the change I made in brainstorm. That leads to have 2 copy of the functions but I guess that a trouble we can accept to still have the code present both in NIRSTORM and Brainstorm repository without any shadowing :)

Which solution do you think is the best @thomas-vincent and @ftadel ? hs. There is also another alternative which is to only keep those 4 functions in nistorm and to force the installation of nirstorm when importing/exporting snirf data but I am pretty sure it's the worst solution haha

thomas-vincent commented 4 years ago

Forking will be better to for usage and maintenance in this case. So option 1. Anyway, changing the convention of how a channel label is built should really not change much in the future. If it does, the change may impact the other code so much that we will have more than just these functions to adapt...

On Tue, May 5, 2020 at 9:03 AM Edouard Delaire notifications@github.com wrote:

I don't have a clear opinion on that subject but I see 3 options here :

-

first, as suggested by Thomas, we don't put the remove the function from the brainstorm external folder and put them and the files where they are called so (out_data_snirf.m and in_data_snirf.m). This solution leads to have 3 copy of the functions.

Remove the function from NIRSTORM repository and keep them in brainstorm. That lead to only one copy of the function but make that the nirstorm code is spread between two repository which can cause trouble is guess (not sure about that, but might)

Leave the two version as is but keep them synchronized. So here we change nst_format_channel accordingly to the change I made in brainstorm. That lead to have 2 copy of the function but I guess that a trouble we can accept to still have the code present both in NIRSTORM and Brainstorm repository.

Which solution do you think is the best @thomas-vincent https://github.com/thomas-vincent and @ftadel https://github.com/ftadel ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Nirstorm/nirstorm/issues/124#issuecomment-624042160, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAE2GDXV7UA5GVU7ZSGPEP3RQAFDXANCNFSM4MY6O6TA .

ftadel commented 4 years ago

Then should I move the 4 nst_* functions from brainstorm3/external/nirstorm to brainstorm3/toolbox/io/private and consider we'll keep working with two separate copies? (this way, it shouldn't even interfere with other versions of the same functions in a different folder in the path)

Make sure you add the extra returned parameter to the nirstorm version, just in case..

Edouard2laire commented 4 years ago

One needs a modification (additional returned parameter): https://github.com/brainstorm-tools/brainstorm3/blob/master/external/nirstorm/nst_format_channel.m

Done in #127 .

Then should I move the 4 nst_* functions from brainstorm3/external/nirstorm to brainstorm3/toolbox/io/private and consider we'll keep working with two separate copies? (this way, it shouldn't even interfere with other versions of the same functions in a different folder in the path)

I don't know. will the snirf importation still work this way ?

ftadel commented 4 years ago

Then should I move the 4 nst_* functions from brainstorm3/external/nirstorm to brainstorm3/toolbox/io/private and consider we'll keep working with two separate copies? (this way, it shouldn't even interfere with other versions of the same functions in a different folder in the path)

I don't know. will the snirf importation still work this way ?

I don't know, this is up to you (@thomas-vincent @Edouard2laire) to decide. In the meantime, I moved these functions toolbox/io/private so that the SNIRF reader in Brainstorm does not interfere with Nirstorm (and vice versa). Just keep in mind that it would be better if these functions are updated in Brainstorm if you modify them in Nirstorm. https://github.com/brainstorm-tools/brainstorm3/commit/1b89539a4d4e7d985818464bde24b218d852b088

Edouard2laire commented 4 years ago

Thx. I am closing this issue then :)

Nirstorm / nirstorm

Integration of the SNIRF reader in Brainstorm #124

first, as suggested by Thomas, we don't put the remove the function from the brainstorm external folder and put them and the files where they are called so (out_data_snirf.m and in_data_snirf.m). This solution leads to have 3 copy of the functions.

Remove the function from NIRSTORM repository and keep them in brainstorm. That lead to only one copy of the function but make that the nirstorm code is spread between two repository which can cause trouble is guess (not sure about that, but might)