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

ENH: Add reverse-ordered transform lists to ants.Registration outputs #3192

Merged
merged 3 commits into from
Mar 30, 2020

Conversation

sulantha2006
Copy link
Contributor

@sulantha2006 sulantha2006 commented Mar 21, 2020

… ANTS Registration outputs.

Summary

Fixes # .

List of changes proposed in this PR (pull-request)

Acknowledgment

  • (Mandatory) I acknowledge that this contribution will be available under the Apache 2 license.

@sulantha2006
Copy link
Contributor Author

Any reason why the doctests are failing here. ?

@codecov
Copy link

codecov bot commented Mar 23, 2020

Codecov Report

Merging #3192 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3192   +/-   ##
=======================================
  Coverage   65.07%   65.08%           
=======================================
  Files         301      301           
  Lines       39624    39628    +4     
  Branches     5247     5247           
=======================================
+ Hits        25786    25790    +4     
  Misses      12771    12771           
  Partials     1067     1067           
Flag Coverage Δ
#unittests 65.08% <100.00%> (+<0.01%) ⬆️
Impacted Files Coverage Δ
nipype/interfaces/ants/registration.py 73.04% <100.00%> (+0.21%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c37fcfb...f6ba809. Read the comment docs.

@SulanthaM
Copy link

SulanthaM commented Mar 24, 2020

Do you think we can incorporate this into pypi soon?
This output reverse_forward_transforms is mentioned in the documentation of NIPYPE antsRegistration but was not implemented to be used. So, if anyone tries to access this output based on the documentation, it fails.

@effigies
Copy link
Member

Can you link to the documentation? It's difficult to review a pull request without context.

@SulanthaM
Copy link

Here is the documentation.

``reverse_forward_transforms`` outputs ``forward_transforms`` in reverse order and can be used for

@effigies
Copy link
Member

Thanks. Judging by the discussion in #2187, it looks like reverse_transforms was deemed to be what was needed, but the docs didn't change...

@Gilles86 Can you confirm?

@SulanthaM
Copy link

reverse_transform is actually a collection of inverse transformations. This corresponds to the reverse_invert_flags as well.
The suggested modification is only a list reverse (reverse the elements in the list) of the forward_transforms list and forward_invert_flags list such that you can feed the output directly to a resampling node without a list reverse function node.

@sulantha2006
Copy link
Contributor Author

Any way we can integrate this sooner to pypi?

@AlanKuurstra
Copy link

I also would appreciate reverse_forward_transforms which is mentioned in the documentation but not implemented. I understand reverse_forward_transforms to be different from reverse_transform.

@effigies effigies changed the title Adding reverse_forward_transforms and reverse_forward_invert_flags to… ENH: Add reverse-ordered transform lists to ants.Registration outputs Mar 30, 2020
Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Seems fine...

@Gilles86 @satra Is this consistent with your understanding?

@satra
Copy link
Member

satra commented Mar 30, 2020

@effigies - seems reasonable from a conceptual standpoint and provides a handy helper to ease inverting lists. i think the docstring still says to use composite transforms when possible.

@effigies effigies merged commit fe15295 into nipy:master Mar 30, 2020
@effigies effigies added this to the 1.5.0 milestone Apr 19, 2020
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.

5 participants