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.

Sign up for GitHub

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

Jump to bottom

Difference between sourcePos and sourcePos3D #41

Closed
Edouard2laire opened this issue Apr 27, 2020 · 20 comments
Closed

Difference between sourcePos and sourcePos3D #41

Edouard2laire opened this issue Apr 27, 2020 · 20 comments

Comments

@Edouard2laire
Copy link
Contributor

Edouard2laire commented Apr 27, 2020 •
edited
Loading

Hi,

I am starting to make brainstorm compatible with snirf (see brainstorm-tools/brainstorm3#283 (comment)) 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
Copy link
Member

fangq commented Apr 27, 2020 •
edited
Loading

@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
Copy link
Collaborator

dboas commented Apr 27, 2020

@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 fangq closed this as completed in 504a7f9 Apr 27, 2020
@fangq fangq reopened this Apr 27, 2020
@fangq
Copy link
Member

fangq commented Apr 27, 2020

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
Copy link
Contributor Author

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
Copy link
Member

fangq commented Apr 27, 2020 •
edited
Loading

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
Copy link
Collaborator

dboas commented Apr 28, 2020 via email

@fangq
Copy link
Member

fangq commented Apr 28, 2020

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
Copy link
Contributor

huppertt commented Apr 28, 2020 via email

fangq added a commit to NeuroJSON/snirf that referenced this issue Apr 29, 2020
@fangq
Rename sourcePos and detectorPos to sourcePos2D/detectorPos2D, close f…
0dd83af 
…NIRS#41
@fangq fangq mentioned this issue Apr 29, 2020
Merged
@fangq
Copy link
Member

fangq commented Apr 29, 2020

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

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

@Edouard2laire
Copy link
Contributor Author

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
Copy link
Member

fangq commented Apr 29, 2020

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:
504a7f9

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
Copy link
Member

fangq commented Apr 30, 2020

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

fangq@1ed004d

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
Copy link
Member

fangq commented Apr 30, 2020

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
Copy link
Contributor Author

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
Copy link
Collaborator

dboas commented May 12, 2020

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 added a commit that referenced this issue May 13, 2020
@fangq
Remove the redundant landMarkPos3D field, #41
6b17719
@fangq fangq closed this as completed in d7c0d2a May 13, 2020
fangq added a commit that referenced this issue May 13, 2020
@fangq
Merge pull request #42 from fangq/master
4858a0c 
Rename sourcePos and detectorPos to sourcePos2D and detectorPos2D, fix #41
@fangq
Copy link
Member

fangq commented May 13, 2020

@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
Copy link
Contributor

huppertt commented May 13, 2020 via email

@huppertt
Copy link
Contributor

huppertt commented May 13, 2020 via email

@fangq
Copy link
Member

fangq commented May 13, 2020

@huppertt, I can revert this change: 6b17719

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 fangq reopened this May 13, 2020
@fangq fangq closed this as completed in 8da44a0 May 16, 2020
@fangq
Copy link
Member

fangq commented May 16, 2020 •
edited
Loading

in the above commit, I made a bunch of clarifications

  • add landmarkPos2D, point landmarkPos as its alias
  • add metadataTags.FrequencyUnit as a new required field for FD related data
  • state that LengthUnit/TimeUnit/FrequencyUnit are case sensitive to distinguish mega from milli
  • clarify on the corresponding units in various time/length fields
  • clarify the data compression filters - deflate is the builtin-compress and lzo/szip are 3rd party - deflate is recommended
  • clarify that wavelengths and wavelengthsEmission are always in nm units
  • clarify that the data dimension are HDF5 dataset dimensions and is language independent
  • changed the sample metadataTags fields to use generic names

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.

@fangq fangq mentioned this issue May 16, 2020
Closed
@HanBnrd HanBnrd mentioned this issue Apr 8, 2021
Closed
@HanBnrd HanBnrd mentioned this issue Apr 16, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@fangq @Edouard2laire @dboas @huppertt