Skip to content

ENH: CompositeTransformUtil, new ANTs interface #2785

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

Merged
merged 3 commits into from
Jan 13, 2019
Merged

Conversation

TheChymera
Copy link
Collaborator

Haven't pushed with make specs, since it edited exclusively files unrelated to this PR.

Acknowledgment

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

@TheChymera TheChymera changed the title ENH: New ANTs interface ENH: CompositeTransformUtil, new ANTs interface Nov 18, 2018
@codecov-io
Copy link

codecov-io commented Nov 18, 2018

Codecov Report

Merging #2785 into master will decrease coverage by 3.4%.
The diff coverage is 68.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2785      +/-   ##
==========================================
- Coverage   67.45%   64.04%   -3.41%     
==========================================
  Files         341      339       -2     
  Lines       43362    43340      -22     
  Branches     5379     5380       +1     
==========================================
- Hits        29248    27757    -1491     
- Misses      13417    14513    +1096     
- Partials      697     1070     +373
Flag Coverage Δ
#smoketests ?
#unittests 64.04% <68.96%> (-0.85%) ⬇️
Impacted Files Coverage Δ
nipype/interfaces/ants/__init__.py 100% <ø> (ø) ⬆️
nipype/interfaces/ants/registration.py 73.92% <68.96%> (-0.3%) ⬇️
nipype/interfaces/nilearn.py 40% <0%> (-56.67%) ⬇️
nipype/utils/spm_docs.py 25.92% <0%> (-44.45%) ⬇️
nipype/interfaces/freesurfer/base.py 50% <0%> (-30.51%) ⬇️
nipype/utils/logger.py 59.7% <0%> (-29.86%) ⬇️
nipype/algorithms/rapidart.py 35% <0%> (-29.42%) ⬇️
nipype/interfaces/spm/base.py 58.08% <0%> (-29.05%) ⬇️
nipype/utils/provenance.py 55.73% <0%> (-28.99%) ⬇️
nipype/interfaces/fsl/model.py 55.26% <0%> (-25.35%) ⬇️
... and 42 more

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 799d3f3...09ae8ed. Read the comment docs.

@effigies effigies added this to the 1.1.7 milestone Nov 26, 2018
@effigies effigies modified the milestones: 1.1.7, 1.1.8 Dec 14, 2018
@TheChymera
Copy link
Collaborator Author

@effigies sorry, I somehow missed the notification :-/

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.

Thanks, this looks good. A few minor suggestions. Also, can you merge master and make specs?

@effigies
Copy link
Member

@TheChymera No worries. Tagging you so this hits your inbox with higher probability.

@TheChymera
Copy link
Collaborator Author

@effigies done. Did not include changes introduced by make specs as they are unrelated to this commit. If you want I can follow up with a separate PR on that.

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.

I merged master and added the autotest from make specs.

Looking at your example, I'm not sure the assemble option will work. It looks like the first in_file in that case is the output file name. If this is the case, this will almost always fail, as there is an existence check. And the outputs (affine_transform, displacement_field) don't really seem valid.

It may make sense to split this out into two separate interfaces (AssembleTransforms and DisassembleTransforms or similar). And it might be worth creating an .h5 file with more than two transforms. I suspect DisassembleTransforms will then produce more than two outputs. And if you assemble in the opposite order (displacement field and then affine), the outputs won't be valid (the 00 and 01 will be switched).

Does that seem correct, or am I misunderstanding the tool? (I haven't used it myself.)

@TheChymera
Copy link
Collaborator Author

@effigies The function is generally used to transition between the newer .h5 format and the old .mat + .nii.gz format in ANTs. So generally these assumptions hold. I agree that a general-purpose solution would be even better, but in that case there's no way to know what transforms went into the file. Is there any way for nipype to just monitor what files were produce and return that list for the outputs?

In any case I don't think splitting this fairly simple function further up would help. At most we can just disable the --disassemble mode until such a time when we can figure out a solution for the output file names which are undefined until you run it.

@effigies
Copy link
Member

effigies commented Jan 2, 2019

At most we can just disable the --disassemble mode until such a time when we can figure out a solution for the output file names

I don't think it works for --assemble at all, though. Just given that the inputs and outputs are so different in the two modes is what made me consider that this might be better as two separate interfaces, called however you like. But in the meantime, maybe we should just make this a --disassemble only interface. And if somebody runs into the case where outputs are different from expected, we can fix it up then.

@TheChymera
Copy link
Collaborator Author

TheChymera commented Jan 2, 2019

@effigies with the new PR --assemble is the one which works. --disassemble also works, but only in the restricted use case. Is there any way to record the names of whatever files the command line interface creates?

@effigies
Copy link
Member

effigies commented Jan 3, 2019

with the new PR --assemble is the one which works

The outputs seem targeted towards disassemble only, with no mention of an assembled output file.

--disassemble also works, but only in the restricted use case

This seems fine, for now. The common case seems to be affine+warp.

Is there any way to record the names of whatever files the command line interface creates?

If you know the pattern, you can use glob. It's not ideal, but when the names/number of outputs depend on the contents of inputs, there's no real getting around it.

@TheChymera
Copy link
Collaborator Author

The outputs seem targeted towards disassemble only, with no mention of an assembled output file.

You're right, I've been editing this in parallel to some other things and lost sight of what I had already done.
I've tested this now again with my common inputs, and it works with for both assembly and disassembly.

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.

LGTM. A couple very minor suggestions. And can you make specs and add the auto tests for this interface?

@TheChymera
Copy link
Collaborator Author

TheChymera commented Jan 9, 2019

make specs again just adds things unrelated to the interface:

diff --git a/nipype/algorithms/tests/test_auto_ACompCor.py b/nipype/algorithms/tests/test_auto_ACompCor.py
index eadbf3e12..235d15da9 100644
--- a/nipype/algorithms/tests/test_auto_ACompCor.py
+++ b/nipype/algorithms/tests/test_auto_ACompCor.py
@@ -6,6 +6,7 @@ from ..confounds import ACompCor
 def test_ACompCor_inputs():
     input_map = dict(
         components_file=dict(usedefault=True, ),
+        failure_mode=dict(usedefault=True, ),
         header_prefix=dict(),
         high_pass_cutoff=dict(usedefault=True, ),
         ignore_initial_volumes=dict(usedefault=True, ),
diff --git a/nipype/algorithms/tests/test_auto_TCompCor.py b/nipype/algorithms/tests/test_auto_TCompCor.py
index 44b01b297..59a5b84f7 100644
--- a/nipype/algorithms/tests/test_auto_TCompCor.py
+++ b/nipype/algorithms/tests/test_auto_TCompCor.py
@@ -6,6 +6,7 @@ from ..confounds import TCompCor
 def test_TCompCor_inputs():
     input_map = dict(
         components_file=dict(usedefault=True, ),
+        failure_mode=dict(usedefault=True, ),
         header_prefix=dict(),
         high_pass_cutoff=dict(usedefault=True, ),
         ignore_initial_volumes=dict(usedefault=True, ),

I can submit a separate PR for that.

Hope this is good now.

@effigies effigies merged commit b491da4 into nipy:master Jan 13, 2019
yarikoptic added a commit to yarikoptic/nipype that referenced this pull request Feb 4, 2019
1.1.8 (January 28, 2019)

  * FIX: ANTS LaplacianThickness cmdline opts fixed up (nipy#2846)
  * FIX: Resolve LinAlgError during SVD (nipy#2838)
  * ENH: Add interfaces wrapping DIPY worflows (nipy#2830)
  * ENH: Update BIDSDataGrabber for pybids 0.7 (nipy#2737)
  * ENH: Add FSL `eddy_quad` interface (nipy#2825)
  * ENH: Support tckgen -select in MRtrix3 v3+ (nipy#2823)
  * ENH: Support for BIDS event files (nipy#2845)
  * ENH: CompositeTransformUtil, new ANTs interface (nipy#2785)
  * RF: Move pytest and pytest-xdist from general requirement into tests_required (nipy#2850)
  * DOC: Add S3DataGrabber example (nipy#2849)
  * DOC: Skip conftest module in API generation (nipy#2852)
  * DOC: Hyperlink DOIs to preferred resolver (nipy#2833)
  * MAINT: Install numpy!=1.16.0 from conda in Docker (nipy#2862)
  * MAINT: Drop pytest-xdist requirement, minimum pytest version  (nipy#2856)
  * MAINT: Disable numpy 1.16.0 for Py2.7 (nipy#2855)

* tag '1.1.8': (79 commits)
  MNT: Add @feilong to .zenodo, update ordering
  MNT: Update .mailmap
  MNT: Update .zenodo ordering
  Accept invitation as Zenodo release co-author (see nipy#2864)
  MAINT: Update .mailmap
  BF: allowing bids_event_file as alternate input
  MNT: Update .zenodo ordering
  MNT: Version 1.1.8
  DOC: 1.1.8 changelog
  Update nipype/interfaces/dipy/tracks.py
  Update nipype/interfaces/dipy/reconstruction.py
  MNT: Install numpy!=1.16.0 from conda in Docker
  Add FSL auto test
  remake specs
  Update nipype/interfaces/io.py
  Remove return type named tuple
  Update nipype/info.py
  STY: Whitespace, line length
  Remove out_ prefix from EddyQuad outputs
  Apply minor edits from code review
  ...
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