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

[Suggestion] Making CoordinateSystem mandatory if Pos3D is present #163

Open
Edouard2laire opened this issue Dec 9, 2024 · 12 comments
Open
Assignees
Labels
enhancement New feature or request

Comments

@Edouard2laire
Copy link
Contributor

Edouard2laire commented Dec 9, 2024

Hello,

Is your suggestion related to a problem? Please describe.
The main objective of having access to Pos3D (sourcePos3D, detectorPos3D) is to be able to coregister the montage with a template or the subject anatomy. This can however become tricky without information about the coordinate system used.

Additionally, the co-registration can be made much easier with access to landmark position (nasion, left/right ear...)

Describe the solution you'd like

Have the coordinate system be mandatory if sourcePos3D and detectorPos3D are present.

Have landmarkPos3D and landmarkLabels be recommended if sourcePos3D and detectorPos3D are present.

Additional context

The description of the coordinate system is required when including nirs data in a BIDS dataset: https://bids-specification.readthedocs.io/en/stable/modality-specific-files/near-infrared-spectroscopy.html#coordinate-system-json-_coordsystemjson

@Edouard2laire Edouard2laire added the enhancement New feature or request label Dec 9, 2024
@Edouard2laire Edouard2laire changed the title [Suggestion] Making landmarkPos3D / CoordinateSystem mandatory if Pos3D is present [Suggestion] Making CoordinateSystem mandatory if Pos3D is present Dec 11, 2024
@dboas
Copy link
Collaborator

dboas commented Jan 2, 2025

@Edouard2laire , I was just looking at this coordinate system requirement in the BIDS spec today. It goes in the coordsystem.json file. I agree this would be good to get into the snirf file. And I like the way you describe the conditional requirement. I believe the validator could handle this language. But I personally wouldn't want to burden ourselves to have the SNIRF validator check if the provided coordinate system is a valid coordinate system since the BIDS validator already handles this. Unless someone volunteered to make that change to the SNIRF validator.

I was thinking that in the use cases in my lab that I would use 'other' for the 'NIRSCoordinateSystem' field and then I would indicate in the 'NIRSCoordinateSystemDescription' field that our landmarks and optodes are in the same coordinate system with mm units.

I wonder what other maintainers and steering committee members think?

@HanBnrd
Copy link
Member

HanBnrd commented Jan 3, 2025

So you're suggesting to turn the /nirs(i)/probe/coordinateSystem or /nirs(i)/probe/landmarkPos3D (which are currently optional) into a required field if 3D positions are present? Could 3D distances simply be used by some users to compute source-detector distances and therefore not necessitate a coordinate system?

I agree it would be better to encourage having it though.

@dboas
Copy link
Collaborator

dboas commented Jan 3, 2025

@HanBnrd , I think the suggestion is making /nirs(i)/probe/coordinateSystem required if 3D positions is present. And recommending, but not requiring /nirs(i)/probe/landmarkPos3D.

One can always using 'other' for the coordinateSystem and indicate that positions are provided in 'mm' units or something like that. So I don't see any issue for those users who aren't aware of the coordinate. In the least, they should be using physical units for the distances if they don't know the coordinate system.

@HanBnrd
Copy link
Member

HanBnrd commented Jan 3, 2025

Sure, I guess I was more thinking in the case where it's made required in snirf specs, and therefore snirf-loading libraries make it impossible to load files that don't have coordinateSystem, people who want to analyse legacy files (eg. from already published open-access datasets) and were just planning to compute distances from 3D pos will not be able to load without tweaking their snirf files. But it's probably a very small use case.

Personally, I'm fine with making it a requirement, it's a good step towards standardisation I think.

@dboas
Copy link
Collaborator

dboas commented Jan 3, 2025

The plan is for the validator to be aware of the snirf version number, and so this should not break older snirf files, provided they have the older version number indicated in the files contents.

@samuelpowell
Copy link
Collaborator

Have the coordinate system be mandatory if sourcePos3D and detectorPos3D are present.

I expect that the vast majority of fNIRS recordings are made in some arbitrary space with landmarks for registration. Most fNIRS software is written for this case and may be unlikely to support 'standard' spaces more commonly encountered in e.g. fMRI. Though of course, the data may move into a standard space during processing.

So I'd support this so long as this 'default' case is easy to specify - I guess this is as you are suggesting @dboas ?

Have landmarkPos3D and landmarkLabels be recommended if sourcePos3D and detectorPos3D are present.

This need is dependent on the specific co-ordinate system? In any case, best to say less than more unless we're enforcing something or guidance is required.

@dboas
Copy link
Collaborator

dboas commented Jan 14, 2025

@samuelpowell , yes, you captured what I am suggesting. Looks like you and I are on the same page.

Do we want to request that someone make the modification to make /nirs(i)/probe/coordinateSystem required if 3D positions is present. And recommending, but not requiring the presence of /nirs(i)/probe/landmarkPos3D? And then do a pull request.

@HanBnrd
Copy link
Member

HanBnrd commented Jan 15, 2025

I can work on the PR

@HanBnrd HanBnrd self-assigned this Feb 3, 2025
@HanBnrd
Copy link
Member

HanBnrd commented Feb 4, 2025

Hi @samuelpowell @dboas,

We met with the maintainers yesterday to chat about issues. It seems this issue would require a change in the validator as currently it's only possible to conditionally require a subfield based on the parent with +, but not another subfield (though please correct me if wrong). This may require a new symbol, potentially a +n? We were not sure however whether this may create conflicts with the simple + at the validator level...

@Edouard2laire
Copy link
Contributor Author

Hello,

If it is difficult to have the field required only if 3D positions are present, what about having it required all the time?
I guess there is no issue in defining a coordinate system, even for 2D position.

@samuelpowell
Copy link
Collaborator

I expect the result will be that the vast majority of SNIRF files will then feature a co-ordinate system specified as 'other'. Perhaps it would be useful to be more specific about the problem that we need to solve here?

@sreekanthkura7
Copy link
Collaborator

@dboas and I discussed this issue. David suggested making /nirs(i)/probe/coordinateSystem a recommended field for now until the validator is updated to work with measurementList and measurementLists.

@dboas, the specifications have so far only designated fields as either optional or required, without using "recommended." However, I believe we can start incorporating this classification.

@sam, should we update the field description to indicate that "other" is the most common option for fNIRS and recommend adding landmarks?

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

No branches or pull requests

5 participants