search

fNIRS / snirf

SNIRF Format Specification
http://fnirs.org/resources/software/snirf/
Other
58 stars 33 forks source link

Difference between sourcePos and sourcePos3D #41

Closed Edouard2laire closed 4 years ago

Edouard2laire commented 4 years ago

Hi,

I am starting to make brainstorm compatible with snirf (see https://github.com/brainstorm-tools/brainstorm3/pull/283#issuecomment-620074533) but I don't understand the difference between sourcePos and sourcePos3D.

SNIRF data format summary said that sourcePos contains 2D pos and sourcePos3D contains 3D pos but then in the specification, it is said that both contains x,y, z coordinates so they both seems to contains 3D pos.

Can someone explain me the difference between the two ? Thanks a lot, Edouard

fangq commented 4 years ago

@Edouard2laire, I agree this was not clear.

I believe the original intent was to use sourcePos and detectorPos to store the 2D optode position in a flattened probe, while sourcePos3D and detectorPos3D to store their 3D positions in world-coordinate.

@dboas, @huppertt and @jayd1860, can you guys confirm if this was the case?

I am happy to update the description to clarify this.

@Edouard2laire, I agree there was a typo in the dimension of sourcePos, and it should be <number of sources> x 2 instead of 3.

dboas commented 4 years ago

@Edouard2laire , @fangq is right. THe intent was for sourcePos to store the 2D coordinates to facilitate viewing of the probe in software packages on a 2D plane. sourcePos3D was then added to provide the true 3D positions of the optodes. We probably should have called sourcePos sourcePos2D instead.

@fangq , can you fix the typo in the spec for sourcePos and I guess also detectorPos to be x 2... but we need to verify with @jayd1860 and @huppertt that that is indeed the case. My feeling is that it might still store x, y, and z.

fangq commented 4 years ago

meant to patch this in my branch, but did it in the main repo, well, please take a look and see if this is clear.

Edouard2laire commented 4 years ago

Thanks for your very quick responses !!

Yes, it is much more clear now. I am still not sure about one part : This field describes the position (in LengthUnit units) of each source optode. The postions can be either coordinates in a flattened 2D probe geometry (z is assumed to be 0), or 3D positions in the world-coordinate system.

I think we should remove 'or 3D positions in the world-coordinate system.` as it implies that sourcePos could store 3D position when, in my understanding, 3D position should only be stored in sourcePos3D.

Therefore, from what I understood from @dboas message, sourcePos could be x 2 as it is not necessary to store 0 but maybe I am wrong.

fangq commented 4 years ago

that was my intent to make sourcePos more flexible - because sourcePos is required and sourcePos3D is optional, if a user happens to have only one of the coordinates (flat or warped), then they can store it with sourcePos. The question is how often people do not have the 2D positions? the downside of this is that it is less specific.

if you all agree with this flexible definition, I can make the column number of sourcePos to be either 2 or 3. Perhaps it was written as 3 because Homer3 assumes it is 3-column originally?

dboas commented 4 years ago

I am a little hesitant to change sourcePos to 2 columns from 2 at this point. But it certainly is more explicit to call it sourcePos2D and make is 2 columns. It is still early in the release. @Qianqian Fangmailto:notifications@github.com do you think it is okay to make this change?

From: Qianqian Fang notifications@github.com Reply-To: fNIRS/snirf reply@reply.github.com Date: Monday, April 27, 2020 at 2:35 PM To: fNIRS/snirf snirf@noreply.github.com Cc: "Boas, David" dboas@bu.edu, Mention mention@noreply.github.com Subject: Re: [fNIRS/snirf] Difference between sourcePos and sourcePos3D (#41)

that was my intent to make sourcePos more flexible - because sourcePos is required and sourcePos3D is optional, if a user happen to have only one of the coordinates (flat or warped), then they can store it with sourcePos. The question is how often people do not have the 2D positions? the downside of this is that it is less specific.

if you all agree with this flexible definition, I can make the column number of sourcePos to be either 2 or 3. Perhaps it was written as 3 because Homer3 assumes it is 3-column originally?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/fNIRS/snirf/issues/41#issuecomment-620159229, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AHFCP5DLNNDFHO4XGQNJSTTROXF75ANCNFSM4MSCLDWQ.

fangq commented 4 years ago

I am a little hesitant to change sourcePos to 2 columns from 2 at this point.

do you mean to change the column number from 3 to 2 for sourcePos?

But it certainly is more explicit to call it sourcePos2D make is 2 columns.

currently sourcePos is a required field, and sourcePos3D is optional. what's your opinion on the requirement states of these two fields if we limit sourcePos to 2D only?

huppertt commented 4 years ago

The naming convention of SrcPos vs SrcPos3D went back to this being how it was done in the SD mat structure in HOMER and the file format for .nirs and NIRx (native) data. In these conventions SrcPos is assumed to be 2D. I’m ok with changing it to SrcPos2D and SrcPos3D to be more explicit, but the use of those terms was a bit historic

Theodore Huppert, PhD University of Pittsburgh Department of Electrical and Computer Engineering

Email: Huppert1@pitt.edu Phone: 1-412-647-8459 Website: www.huppertlab.net

From: Qianqian Fang notifications@github.com Sent: Tuesday, April 28, 2020 8:55:45 AM To: fNIRS/snirf snirf@noreply.github.com Cc: huppertt dr.huppert@gmail.com; Mention mention@noreply.github.com Subject: Re: [fNIRS/snirf] Difference between sourcePos and sourcePos3D (#41)

I am a little hesitant to change sourcePos to 2 columns from 2 at this point.

do you mean to change the column number from 3 to 2 for sourcePos?

But it certainly is more explicit to call it sourcePos2D make is 2 columns.

currently sourcePos is a required field, and sourcePos3D is optional. what's your opinion on the requirement states of these two fields if we limit sourcePos to 2D only?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/fNIRS/snirf/issues/41#issuecomment-620588462, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ALNU3OQO4HE4AYZOQ6WZG73RO3G5DANCNFSM4MSCLDWQ.

fangq commented 4 years ago

let me know if this edit clears things up, if all agree, I will merge

https://github.com/fNIRS/snirf/pull/42/files

Edouard2laire commented 4 years ago

Hi @fangq,

it looks great to me. Maybe we could use this opportunity to also clarify the role of landmarkPos and landmarkPos3D. In this case, I am wondering if there is really a need to have them both. Is there people who have digitalized head point, but only in 2D coordinate ?

I also have a naïve question, (sorry if it's obvious), shall we be worried about backward compatibility, especially from a BIDS point a view ? especially, shall we add code in out different software to keep reading sourcePos and transfert it to sourcePos2D ?

fangq commented 4 years ago

I also have a naïve question, (sorry if it's obvious), shall we be worried about backward compatibility, especially from a BIDS point a view ? especially, shall we add code in out different software to keep reading sourcePos and transfert it to sourcePos2D ?

Keeping the spec backward-compatible was my main thought when I made the previous commit: https://github.com/fNIRS/snirf/commit/504a7f99f0a11bd62172931b5860bfecc8c6cee3

I always hate to break backward compatibility because it immediately renders all existing code incompatible. But if the spec is still in the early stage and such change is unavoidable, it is probably ok. I will let @dboas to make the call.

fangq commented 4 years ago

@Edouard2laire, see my updated pull request, especially this commit

https://github.com/fangq/snirf/commit/1ed004dcb3ad3cffe340ae578252c8051f92d6c8

I added sourcePos and detectorPos back, and claim them as the alias to ...Pos2D.

the issue is that

  1. we need to prevent people from using both
  2. previously, sourcePos/detectorPos were required, now ...Pos2D tags are required. Not sure if this is the best way to handle these for better backward compatibility.
fangq commented 4 years ago

it looks great to me. Maybe we could use this opportunity to also clarify the role of landmarkPos and landmarkPos3D. In this case, I am wondering if there is really a need to have them both. Is there people who have digitalized head point, but only in 2D coordinate ?

The two sections look identical

https://github.com/fNIRS/snirf/blob/master/snirf_specification.md#nirsiprobelandmarkpos

maybe make landmarkPos3D as the default, and make landmarkPos as an alias?

Edouard2laire commented 4 years ago

Hi, I am not sure if you are waiting an answer from me; but in case, all those changes are ok for me :)

hs : did you had time to have a look at my question on snirf sample ? https://github.com/fNIRS/snirf-samples/issues

dboas commented 4 years ago

Jay and I are discussing this now and agree with the following:

We are all for sourcePos2D and detectorPos2D as being required and they should be a matrix with 2 columns. We can keep sourcePos and detectorPos as optional for historical reasons and in the description indicate they are kept for historical reasons but developers are encouraged to copy the variables to the replacement sourcePos2D and detectorPos2D and remove sourcePos and detectorPos from the SNIRF file.

We think we can remove landmarkPos3D since it is a duplication of landmarkPos. I think this happened this way because it just copied what we did with sourcePos and sourcePos3D.

@fangq , if you agree, can you make the change?

fangq commented 4 years ago

@dboas, the duplicated landmarkPos3D field is removed, and the edits regarding sourcePos2D/3D etc are merged with the master repo. see all changes at

https://github.com/fNIRS/snirf/pull/42/files

I kept the sourcePos and detectorPos for compatibility, but recommend users to use the 2D versions of the keywords.

Feel free to let me know if you see any remaining problems.

huppertt commented 4 years ago

The landmark3d was intended to (obviously) be the 3D version but the landmark was the 2D version. You would need to add landmark2d instead and if you leave the source and detector (not explicitly 3d or 2d), you need to leave the landmark as well.

Theodore Huppert, PhD University of Pittsburgh Department of Electrical and Computer Engineering

Email: Huppert1@pitt.edu Phone: 1-412-647-8459 Website: www.huppertlab.net

From: Qianqian Fang notifications@github.com Sent: Wednesday, May 13, 2020 9:48:08 AM To: fNIRS/snirf snirf@noreply.github.com Cc: huppertt dr.huppert@gmail.com; Mention mention@noreply.github.com Subject: Re: [fNIRS/snirf] Difference between sourcePos and sourcePos3D (#41)

@dboashttps://github.com/dboas, the duplicated landmarkPos3D field is removed, and the edits regarding sourcePos2D/3D etc are merged with the master repo. see all changes at

https://github.com/fNIRS/snirf/pull/42/files

I kept the sourcePos and detectorPos for compatibility, but recommend users to use the 2D versions of the keywords.

Feel free to let me know if you see any remaining problems.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/fNIRS/snirf/issues/41#issuecomment-627998382, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ALNU3OU435H4H5OIJDTANBTRRKQJRANCNFSM4MSCLDWQ.

huppertt commented 4 years ago

And for clarification, you need a 2D version of landmarks to do registration (eg the way anchors get used in atlas viewer). This is needed in the snirf format to accommodate data from NIRx

Theodore Huppert, PhD University of Pittsburgh Department of Electrical and Computer Engineering

Email: Huppert1@pitt.edu Phone: 1-412-647-8459 Website: www.huppertlab.net

From: Theodore Huppert dr.huppert@gmail.com Sent: Wednesday, May 13, 2020 4:02:11 PM To: fNIRS/snirf reply@reply.github.com; fNIRS/snirf snirf@noreply.github.com Cc: Mention mention@noreply.github.com Subject: Re: [fNIRS/snirf] Difference between sourcePos and sourcePos3D (#41)

The landmark3d was intended to (obviously) be the 3D version but the landmark was the 2D version. You would need to add landmark2d instead and if you leave the source and detector (not explicitly 3d or 2d), you need to leave the landmark as well.

Theodore Huppert, PhD University of Pittsburgh Department of Electrical and Computer Engineering

Email: Huppert1@pitt.edu Phone: 1-412-647-8459 Website: www.huppertlab.net

From: Qianqian Fang notifications@github.com Sent: Wednesday, May 13, 2020 9:48:08 AM To: fNIRS/snirf snirf@noreply.github.com Cc: huppertt dr.huppert@gmail.com; Mention mention@noreply.github.com Subject: Re: [fNIRS/snirf] Difference between sourcePos and sourcePos3D (#41)

@dboashttps://github.com/dboas, the duplicated landmarkPos3D field is removed, and the edits regarding sourcePos2D/3D etc are merged with the master repo. see all changes at

https://github.com/fNIRS/snirf/pull/42/files

I kept the sourcePos and detectorPos for compatibility, but recommend users to use the 2D versions of the keywords.

Feel free to let me know if you see any remaining problems.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/fNIRS/snirf/issues/41#issuecomment-627998382, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ALNU3OU435H4H5OIJDTANBTRRKQJRANCNFSM4MSCLDWQ.

fangq commented 4 years ago

@huppertt, I can revert this change: https://github.com/fNIRS/snirf/commit/6b177194cce75c1d7130857cad88d87882e9121b

and rename landmarkPos to landmarkPos2D, then add landmarkPos as an alias to the 2D version?

please confirm if this is what you suggest me to do.

also, can you explain how is landmarkPos2D computed? just out of my curiosity.

fangq commented 4 years ago

in the above commit, I made a bunch of clarifications

the only potential issue is that when someone writes a landmarkPos array with 3 columns of data - according to this commit, the last column is supposed to be the label index, but people may misunderstand it as landmarkPos3D.

Please take a look and see if any other issues that you need further clarifications. Feel free to reopen if further edits is needed.