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

[BIDS] import NIRS dataset (fixes #469) #589

Merged
merged 13 commits into from
Nov 21, 2022

Conversation

Edouard2laire
Copy link
Contributor

@Edouard2laire Edouard2laire commented Nov 14, 2022

Hello,

This is finally implementing the BIDS importation for NIRS :).

Two datasets can be found here:

@Edouard2laire Edouard2laire mentioned this pull request Nov 14, 2022
@Edouard2laire Edouard2laire marked this pull request as ready for review November 14, 2022 19:04
@Edouard2laire
Copy link
Contributor Author

Edouard2laire commented Nov 14, 2022

ok. so I broke everything that last commit....

Capture d’écran 2022-11-14 à 17 35 55

Left: When importing the .snirf, Right when using coordinate from the optodes.txt file. (left is the correct montage covering both motor cortices)

If I do right click on the channel file, and then import from a file then all the optodes get pulled to the middle of the brain

@Edouard2laire
Copy link
Contributor Author

Edouard2laire commented Nov 14, 2022

Capture d’écran 2022-11-14 à 17 51 20

It seems there is some kind of 90 degree rotation between the real montage and what we get.

So the json file is saying:

{
    "NIRSCoordinateSystem": "CapTrak",
    "NIRSCoordinateSystemDescription": "The X-axis goes from the left preauricular point (LPA) through the right preauricular point (RPA). The Y-axis goes orthogonally to the X-axis through the nasion (NAS). The Z-axis goes orthogonally to the XY-plane through the vertex of the head. This corresponds to a \"RAS\" orientation with the origin of the coordinate system approximately between the ears. See Appendix VIII in the BIDS specification.",
    "NIRSCoordinateUnits": "m"
}

So I guess that the coordinate system in Brainstorm should be ScanRAS. They don't provide the fiducials outside the snirf file so I don't know what to do. If I specify electrodesSpace = 'ACPC'; it is the good orientation, but the electrodes are floating ~3cm above the head.

@Edouard2laire
Copy link
Contributor Author

Edouard2laire commented Nov 15, 2022

If I change the json file to :

{
    "NIRSCoordinateSystem": "CapTrak",
    "NIRSCoordinateSystemDescription": "The X-axis goes from the left preauricular point (LPA) through the right preauricular point (RPA). The Y-axis goes orthogonally to the X-axis through the nasion (NAS). The Z-axis goes orthogonally to the XY-plane through the vertex of the head. This corresponds to a \"RAS\" orientation with the origin of the coordinate system approximately between the ears. See Appendix VIII in the BIDS specification.",
    "NIRSCoordinateUnits": "m",
    "FiducialsCoordinates": {
        "LPA":[-0.0824899918305801,3.5272652819384742E-9,-8.9852658646805139E-10],
        "NAS":[6.5340601852759139E-12,0.11404663614484922,-8.9566691563458534E-9],
        "RPA":[0.082489996979284677,3.8930906380574282E-9,4.766247813092761E-10] },
    "FiducialsCoordinateUnits": "m"
}

where the fiducials comes from the snirf file then the coregistration is ok. would it be possible to take the fiducials automatically from the snirf file when they are not in the json file ?

Edit: actually, should we ignore the optodes.tsv and _coordsystem.json files since they are just a copy of what is inside the snirf file with less information?

@ftadel ftadel changed the title [BIDS] import NIRS dataset [BIDS] import NIRS dataset (fixes #469) Nov 15, 2022
@ftadel
Copy link
Member

ftadel commented Nov 15, 2022

actually, should we ignore the optodes.tsv and _coordsystem.json files since they are just a copy of what is inside the snirf file with less information?

In the BIDS logic:

  • Some information missing in the raw data can be added in the metadata
  • If there are multiple conflicting information in the dataset (e.g. 3D coordinates different in the raw files and in the BIDS metadata), then then BIDS metadata should be used.
  • However, at the same time, the metadata SHOULD NOT conflict with the raw data files.

This is a complicated topic, actively debated: bids-standard/bids-specification#761
But there is a consensus that: no, we should not always ignore the metadata.
I think we need to keep this logic: first importing the .snirf file, then if there is additional information available in the metadata, import it.
But then, it must be imported correctly...

In the past weeks, I spent a lot of time spotting inconsistencies in public datasets and reporting them on OpenNeuro...
I noticed that most datasets using the "CapTrak" coordinate system, but without reporting the actual positions of the NAS/RPA/NAS/Vertex fiducials, while they are necessary to interpret the coordinates correctly...
These fiducials should be reported both in the "CapTrak" system itself, and in the anatomy if present.
This is not captured by the validator, but maybe it should at least when an anatomy is present. Without the coordinates of the fiducials in the T1w of the subject, we can't align the cap on the anatomy.

Brainstorm should also include a solution for converting to and from "CapTrak", in cs_compute.m and cs_convert.m, in the same way I implemented the ACPC coordinates, in order to align correctly at least on the MNI anatomy...
I'll work on this.
In the meantime, you can: add the fiducials in the metadata manually, and report to the authors of the dataset that it would be better to include them in there dataset.

where the fiducials comes from the snirf file then the coregistration is ok. would it be possible to take the fiducials automatically from the snirf file when they are not in the json file ?

No, because they are not necessarily in the same coordinate system...

toolbox/io/in_data_snirf.m Outdated Show resolved Hide resolved
toolbox/tree/tree_callbacks.m Show resolved Hide resolved
toolbox/sensors/channel_add_loc.m Outdated Show resolved Hide resolved
toolbox/sensors/channel_add_loc.m Outdated Show resolved Hide resolved
toolbox/process/functions/process_import_bids.m Outdated Show resolved Hide resolved
toolbox/process/functions/process_import_bids.m Outdated Show resolved Hide resolved
toolbox/process/functions/process_import_bids.m Outdated Show resolved Hide resolved
toolbox/process/functions/process_import_bids.m Outdated Show resolved Hide resolved
@Edouard2laire
Copy link
Contributor Author

Thanks a lot for the feedbacks. I will work on them during the weekend :)

@ftadel
Copy link
Member

ftadel commented Nov 18, 2022

I committed some new code to handle the CapTrak coordinates from BIDS datasets:
b3f2550

@Edouard2laire
Copy link
Contributor Author

Edouard2laire commented Nov 18, 2022

thx. By any chance, do you think you could manage to merge the changes on this branch so I can keep working on it without having any issue with merging conflicts ? :) edit : I found a way :)

@Edouard2laire
Copy link
Contributor Author

Edouard2laire commented Nov 20, 2022

Ok; I think it is ready for review. By the way, shouldn't the process import_bids have the option to create a new protocol instead of adding the data to the current protocol by default?

Maybe something like :

    % ===== IMPORT FILES =====

    try
        json = bst_jsondecode(fullfile( BidsDir,'dataset_description.json'));
        ProtocolName = json.Name;
    catch
        disp(['BIDS> Error: Cannot read json file: dataset_description.json']);
        sProtocol = bst_get('ProtocolInfo');
        ProtocolName = sProtocol.Comment;
   end

    if isempty(bst_get('Protocol', ProtocolName))
        gui_brainstorm('CreateProtocol', ProtocolName, 0, 0);
    end

@ftadel
Copy link
Member

ftadel commented Nov 21, 2022

By the way, shouldn't the process import_bids have the option to create a new protocol instead of adding the data to the current protocol by default?

I'm not sure the dataset name is really a good solution: it is sometimes empty, sometimes very long... not very adapted to be a folder name in the brainstorm_db folder.

However, it's indeed not very intuitive that the dataset is going to be imported in the current protocol.
It's something that we want to make possible (new subjects or new data added to an existing study), but not enforce.

I think I will add a new option "Protocol name (empty=current protocol)" in the process options.
At least it makes it clear that by default, it imports in the current protocol.
Do you have a better idea?

If a channel has two locations, I don't think we need to make the test for both locations.
Testing one, and it not there, adding the two points.
Much more readable :)
@ftadel
Copy link
Member

ftadel commented Nov 21, 2022

Good job! :)

@ftadel ftadel merged commit 7b0128b into brainstorm-tools:master Nov 21, 2022
@Edouard2laire Edouard2laire deleted the BIDS-nirs2 branch November 21, 2022 18:37
@Edouard2laire
Copy link
Contributor Author

Edouard2laire commented Nov 22, 2022

Good job! :)

Thank you. Should we tweet about it (maybe as response to https://mobile.twitter.com/BIDSstandard/status/1586327298608955392 ) ? We can add it to the updates.txt too :)

I'm not sure the dataset name is really a good solution: it is sometimes empty, sometimes very long... not very adapted to be a folder name in the brainstorm_db folder.

Yes, you are right. It seems that the datasets filling those metadata carefully are quite rare...

However, it's indeed not very intuitive that the dataset is going to be imported in the current protocol. It's something that we want to make possible (new subjects or new data added to an existing study), but not enforce.

I think I will add a new option "Protocol name (empty=current protocol)" in the process options. At least it makes it clear that by default, it imports in the current protocol. Do you have a better idea?

yes, i have some ideas but need a bit of time to clarify them in my head before writting them here :) Will try to describe them here before the end of the week :)

@ftadel
Copy link
Member

ftadel commented Nov 22, 2022

Should we tweet about it (maybe as response to https://mobile.twitter.com/BIDSstandard/status/1586327298608955392 ) ?

Sure! Please feel free to do it.
Just include @brainstorm2day.

We can add it to the updates.txt too :)

Yes, I forgot, it will be in my next commits.

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.

2 participants