afids / afids-validator

Validator for the anatomical fiducial placement protocol
https://validator.afids.io
GNU General Public License v3.0
2 stars 4 forks source link

support for 3D Slicer v4.11 and beyond #72

Closed jclauneuro closed 4 years ago

jclauneuro commented 4 years ago

The syntax for headers in 3D Slicer v4.11 has changed resulting in some erroneous behaviour if loading files into 3D Slicer with an earlier version (images in the same space are flipped relative to each other -- one is RAS and the other LPS).

Screen Shot 2020-04-06 at 8 36 47 PM

Some details here: https://discourse.slicer.org/t/converting-fiducial-coordinates-from-ras-to-lps/9707/6

For now, the simple thing to do is enforce the use of 4.10.2 for afids annotations (we've changed all the documentation accordingly). At some point (time permitting), dealing with the change in header can be dealt with by the validator.

kaitj commented 4 years ago

Do we have an available fcsv for 4.11+? I think we can add in in a sign flip by just checking the version for the fcsv (which is already being done anyways).

@tkkuehn thinking ahead, do we need additional unit test(s)?

kaitj commented 4 years ago

Addressed the flip, with changes currently on the afid-flip branch (https://github.com/afids/afids-validator/tree/afid-flip).

Going to wait to see if we want to include additional unit testing and if we have an availabe 4.11+ fcsv for creating a PR.

jclauneuro commented 4 years ago

In the long term, this change in specification may be a good thing but for now has the possibility of breaking scripts or having unintentional consequences (image flipping) if header data is not appropriately accounted for.

Uploaded the examples via Slack and also here as txt files. To complete the examples for testing purposes, would probably be good to have an example CoordinateSystem "1" in version 4.10.2 and CoordinateSystem "RAS" in 4.11 (with all coordinates meant to define the same space).

Fid32_macaqueMNI_T1_Slicer_v4.10.2.txt Fid32_macaqueMNI_T1_Slicer_v4.11.txt

kaitj commented 4 years ago

Does Slicer allow you to choose the coordinate system or does it still only make use of a single coordinate system and they've just changed the default?

At this point, we haven't been making use of the coordinate system line in the file, rather just checking the version. If there are multiple coordinate systems that can be used in a single Slicer version, we can add something to take into account the coordinate system.

jclauneuro commented 4 years ago

I just checked in v4.10.2 and there is no easy way to change the coordinate system although I think with some digging there would be a way. At some point it may be worth figuring out which coordinate system (LPS, RAS, ...) is being input based on the header and calculating based on that. Alternatively, a specific coordinate system could be enforced and others deemed invalid. Not sure there is a correct answer to this and worth discussing at some point.

kaitj commented 4 years ago

Yeah, would be a good discussion point. Was thinking if it is something that is easy to change around, then we should consider the coordinate system when we validate, otherwise, we can assume the coordinate system used by Slicer based on version.

kaitj commented 4 years ago

Adding notes from the previous meeting:

ACTION ITEMS

jclauneuro commented 4 years ago

Was reminded about this at the afids-validator meeting today. Any updates on this @greydongilmore?

jclauneuro commented 4 years ago

Update: @greydongilmore has looked into this. See thread on discourse: https://discourse.slicer.org/t/user-setting-for-coordinate-system-in-v4-11/11567

To summarize:

Slicer v5.0 will save Markup files in LPS coordinate system by default. Up until v4.10.2, RAS has been the default output coordinate system. We started noticing issues when AFIDs placements in v4.11 were displayed in versions < 4.11.

The switch to LPS is probably a good thing in the long term so that coordinates do not need to be converted from RAS-->LPS, which is the standard for most neuroimaging applications. However, in the short term this may create some conflicts with existing AFIDs workflows.

Fortunately, the AFIDs descriptors have built in L/R encoding of features (in the name and description) which can be used for creating test/validation cases. Note: info about the Slicer version and coordinate system are included in the header which can be parsed accordingly.

In the meantime, we will continue to ask users not to use v4.11 or above.

I will ask Andras at Slicer about when they anticipate v5.0 being released so we have a rough timeline for implementing this check in the validator.

kaitj commented 4 years ago

That is good to know. I have just drafted a PR (#81) that contained a fix made a few weeks ago. To summarize the PR, the version is checked from the header, and if it is greater than or equal to v4.11, flip the xand y coordinate of each fiducial. And this is true for both user uploaded files and the templates.

Just want to add in a unit test before finalizing the PR.

kaitj commented 4 years ago

This was addressed with merge, and closing!