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

Make Affine take RAS parameters of floating->ref #712

Merged
merged 3 commits into from Oct 25, 2021

Conversation

fepegar
Copy link
Owner

@fepegar fepegar commented Oct 24, 2021

Description
Previous behavior: the input parameters were assumed to use SimpleITK conventions and to build a transformation from reference to floating space.
Current behavior: the input parameters are assumed to not use SimpleITK conventions (for example, an image RAS with translation 10, 20, 30 will move 10 mm to the right, 20 towards the front and 30 upwards) and to build a transformation from floating to reference space. This is much more intuitive.

Checklist

  • I have read the CONTRIBUTING docs and have a developer setup (especially important are pre-commitand pytest)
  • Non-breaking change (would not break existing functionality)
  • Breaking change (would cause existing functionality to change)
  • Tests added or modified to cover the changes
  • Integration tests passed locally by running pytest
  • In-line docstrings updated
  • Documentation updated, tested running make html inside the docs/ folder
  • This pull request is ready to be reviewed
  • If the PR is ready and there are multiple commits, I have squashed them and force-pushed

@fepegar fepegar marked this pull request as ready for review October 24, 2021 19:41
@romainVala
Copy link
Contributor

for example, an image RAS with translation 10, 20, 30 will move 10 mm

Just to point out, that this "frame orientation" dependence of the affine transforms is not related at all the the RAS or LPS convention of the nifti image. (It may be related to the intrinsic representation of the image )
So just to be clear, the same will apply wether you have a RAS or LPS nifti image ...

we must be carefull to not add confusion to the confusing issue ...

Having say that, great job here, I was asking myself this morning if I should ask you an update on my #693 issue, but you solve it in a dunday afternoon !

many thanks

Copy link
Contributor

@romainVala romainVala left a comment

Choose a reason for hiding this comment

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

Many thanks
let's make this orientation stuff, less confusing !!!

@fepegar
Copy link
Owner Author

fepegar commented Oct 25, 2021

Just to point out, that this "frame orientation" dependence of the affine transforms is not related at all the the RAS or LPS convention of the nifti image. (It may be related to the intrinsic representation of the image )
So just to be clear, the same will apply wether you have a RAS or LPS nifti image ...
we must be carefull to not add confusion to the confusing issue ...

In this case, I used RAS just as an example. I can maybe add one more example, slightly more complex. I think I might have added something to the docstring and forgot to commit. I will take a look tonight.

Having say that, great job here, I was asking myself this morning if I should ask you an update on my #693 issue, but you solve it in a Sunday afternoon !

The only time I have left to work on this library, pretty much :)

many thanks

Thank you for reviewing!

@fepegar
Copy link
Owner Author

fepegar commented Oct 25, 2021

I think I might have added something to the docstring and forgot to commit. I will take a look tonight.

I actually had added the comment in master.

@fepegar fepegar merged commit 8490339 into master Oct 25, 2021
@fepegar fepegar deleted the 710-fix-affine-inverse branch October 25, 2021 09:59
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.

Affine transform is applying the inverse matrix
2 participants