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

FIX: Fix slice orientations #121

Merged
merged 4 commits into from
Apr 5, 2021
Merged

Conversation

kaitj
Copy link
Collaborator

@kaitj kaitj commented Apr 3, 2021

Resolves #118 - flipping the rotations in the yaw & roll in the sagittal view seems to have resolved this. Views should now be "Sagittal right" and "Coronal anterior" The rotations applied here was used to produce the figure in #120.

@netlify
Copy link

netlify bot commented Apr 3, 2021

Deploy preview for carpentries-dmri ready!

Built with commit e78b80e

https://deploy-preview-121--carpentries-dmri.netlify.app

@jhlegarreta
Copy link
Contributor

Checked locally. A few comments:

  • Are we sure that the coronal anterior is a coronal anterior? Looking at it I'd say that it's a coronal posterior; looking at the sagittal, things get a little bit more confusing.
  • Fixing the coronal by setting the sagittal means that the fix would work if we call the methods in a given order. We should reset the camera in each method and then perform the appropriate changes to it so that it does not depend on the changes that were applied previously.

I think all concerned figures should be updated within this PR.

Also, I've realized about a mistake of mine in PR #92 and have submitted PR #122, so prior to generating all figures, this branch should be rebased on master once the latter PR gets merged. My bad. Sorry @kaitj .

@kaitj
Copy link
Collaborator Author

kaitj commented Apr 4, 2021

* Are we sure that the coronal anterior is a coronal anterior? Looking at it I'd say that it's a coronal posterior; looking at the sagittal, things get a little bit more confusing.

Ahh yeah, took another look, you are right. 29e2e6b should fix this to coronal anterior (pitch changed to -90)

* Fixing the coronal by setting the sagittal means that the fix would work if we call the methods in a given order. We should reset the camera in each method and then perform the appropriate changes to it so that it does not depend on the changes that were applied previously.

Yeah, I noticed that that too. I think for now, for the purposes of generating the figures in the correct orientations, this is probably temporarily fine, but definitely want to look into resetting the camera as a more permanent solution.

Also, I've realized about a mistake of mine in PR #92 and have submitted PR #122, so prior to generating all figures, this branch should be rebased on master once the latter PR gets merged. My bad. Sorry @kaitj .

No worries!

@jhlegarreta
Copy link
Contributor

I've merged #122. We can rebase this on master and hopefully get all figures generated correctly within this PR.

@kaitj kaitj merged commit 45be79b into carpentries-incubator:master Apr 5, 2021
@kaitj kaitj deleted the update_slices branch April 5, 2021 14:22
@jhlegarreta
Copy link
Contributor

Figures will need to be updated in a separate PR then.

@kaitj kaitj mentioned this pull request Apr 5, 2021
3 tasks
@kaitj
Copy link
Collaborator Author

kaitj commented Apr 5, 2021

Yes, I created a new issue to keep track of this. Merged this in as I had already rebased my local master and it would have gotten merged in with #130 otherwise.

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.

Fix the coronal and sagittal views of the peaks/tractograms
2 participants