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

Transformation Update #113

Merged
merged 4 commits into from
Aug 5, 2024

Conversation

NicerNewerCar
Copy link
Contributor

@NicerNewerCar NicerNewerCar commented Jul 24, 2024

  • Adds DICOM2AUT tfm
  • Splits {bone}.tfm into {bone}_t.tfm and {bone}_scale.tfm
  • Renames and addresses issues with Slicer2Autoscoper transform (now PVOL2AUT)

@NicerNewerCar NicerNewerCar requested review from jcfr and amymmorton July 24, 2024 17:26
@NicerNewerCar NicerNewerCar marked this pull request as ready for review July 24, 2024 17:27
@amymmorton
Copy link
Contributor

building now... and will confirm, but a comment on the commit message re: splitting the .tfm:
We do want all three variants: bone.tf boneScaleOnly.tfm and boneTransOnly.tfm

@NicerNewerCar
Copy link
Contributor Author

building now... and will confirm, but a comment on the commit message re: splitting the .tfm: We do want all three variants: bone.tf boneScaleOnly.tfm and boneTransOnly.tfm

Okay, I will update this now to include the {bone}.tfm again

@amymmorton
Copy link
Contributor

amymmorton commented Jul 25, 2024

  • Pvol 2 aut

suggest

  • export of the AUT(bone).stl (LPS)
  • change bone_t to inv(bone_t)
  • the DICOM2AUT tfm is combined transform of the image position to aut AND RAS->LPS - suggest naming explicitly? (if user segments in mimics and applies this tform- (staying LPS) to the bone model or a cs gen from the anatomy it will be erroneous.

@NicerNewerCar
Copy link
Contributor Author

  • change bone_t to inv(bone_t)

Should this also apply to the naming of the bone_scale (ie. inv(bone_scale))

  • the DICOM2AUT tfm is combined transform of the image position to aut AND RAS->LPS - suggest naming explicitly? (if user segments in mimics and applies this tform- (staying LPS) to the bone model or a cs gen from the anatomy it will be erroneous.

Would something like DICOM(RAS)2AUT(LPS) work or were you thinking of something else?

@NicerNewerCar NicerNewerCar force-pushed the transforms-edit branch 3 times, most recently from d4a3837 to e89a735 Compare July 25, 2024 17:24
@@ -757,30 +767,29 @@ def onLoadPV(self):
transformSubDir = self.ui.tfmSubDir.text

vols = glob.glob(os.path.join(mainOutputDir, volumeSubDir, "*.tif"))
tfms = glob.glob(os.path.join(mainOutputDir, transformSubDir, "*.tfm"))
tfms_t = glob.glob(os.path.join(mainOutputDir, transformSubDir, "*_t.tfm"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independently of this pull request, we should consider documentating the different transform files generated by Autoscoper/AutoscoperM.

May be by updating the following section: https://autoscoper.readthedocs.io/en/latest/file-specifications/index.html

Copy link
Contributor

@jcfr jcfr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After addressing few nitpicks and confirming there are no regression, unless @amymmorton has additional comments, this is good for integration 🚀

@amymmorton
Copy link
Contributor

amymmorton commented Jul 31, 2024

The bone_t.tfm are in RAS not LPS (name it explicitly?)
(ground truth data prev sent 7/16 vs this branch)
image

  • GT AUT mc1 stl and AUT stl generated in this branch are spatially aligned
  • mc1-DICOM2AUT.tfm transforms the segmented mc1 stl to AUT space in RAS -slicer

the output tra (with LPS-RAS) tform applied is correct!

image
image

@NicerNewerCar NicerNewerCar merged commit f689073 into BrownBiomechanics:main Aug 5, 2024
2 checks passed
@NicerNewerCar NicerNewerCar deleted the transforms-edit branch August 5, 2024 16:11
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.

3 participants