-
Notifications
You must be signed in to change notification settings - Fork 11
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
Integration of the SNIRF reader in Brainstorm #124
Comments
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. |
The modified function from Brainstorm would work with Nirstorm (it never hurts to add returned parameters). |
It is better to isolate everything needed to load snirf in brainstorm and not depend on nirstorm. If you really need to share the functions, then the functions in question should be moved in brainstorm. |
I don't have a clear opinion on that subject but I see 3 options here :
Which solution do you think is the best @thomas-vincent and @ftadel ? |
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 ***@***.***> 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
<#124 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE2GDXV7UA5GVU7ZSGPEP3RQAFDXANCNFSM4MY6O6TA>
.
|
Then should I move the 4 nst_* functions from Make sure you add the extra returned parameter to the nirstorm version, just in case.. |
Done in #127 .
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. |
Thx. I am closing this issue then :) |
@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?
The text was updated successfully, but these errors were encountered: