Skip to content

ENH: Flesh out ConcatenateLTA #2215

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 6, 2017
Merged

ENH: Flesh out ConcatenateLTA #2215

merged 1 commit into from
Oct 6, 2017

Conversation

effigies
Copy link
Member

@effigies effigies commented Oct 6, 2017

Changes proposed in this pull request

  • Adds most options to mri_concatenate_lta (skipping -rmsdiff because it's a weird one and I don't need it)
  • Update existing fields to match tool specs (see especially in_lta2)

@codecov-io
Copy link

Codecov Report

Merging #2215 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2215      +/-   ##
=========================================
+ Coverage    72.3%   72.3%   +<.01%     
=========================================
  Files        1178    1178              
  Lines       58736   58747      +11     
  Branches     8456    8457       +1     
=========================================
+ Hits        42468   42477       +9     
- Misses      14896   14898       +2     
  Partials     1372    1372
Flag Coverage Δ
#smoketests 72.3% <100%> (ø) ⬆️
#unittests 69.86% <100%> (ø) ⬆️
Impacted Files Coverage Δ
...faces/freesurfer/tests/test_auto_ConcatenateLTA.py 85.71% <ø> (ø) ⬆️
nipype/interfaces/freesurfer/preprocess.py 65.64% <100%> (+0.37%) ⬆️
nipype/pipeline/plugins/multiproc.py 76.47% <0%> (-1.48%) ⬇️

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 4d08138...1bd5a02. Read the comment docs.

@codecov-io
Copy link

Codecov Report

Merging #2215 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2215      +/-   ##
=========================================
+ Coverage    72.3%   72.3%   +<.01%     
=========================================
  Files        1178    1178              
  Lines       58736   58747      +11     
  Branches     8456    8457       +1     
=========================================
+ Hits        42468   42477       +9     
- Misses      14896   14898       +2     
  Partials     1372    1372
Flag Coverage Δ
#smoketests 72.3% <100%> (ø) ⬆️
#unittests 69.86% <100%> (ø) ⬆️
Impacted Files Coverage Δ
...faces/freesurfer/tests/test_auto_ConcatenateLTA.py 85.71% <ø> (ø) ⬆️
nipype/interfaces/freesurfer/preprocess.py 65.64% <100%> (+0.37%) ⬆️
nipype/pipeline/plugins/multiproc.py 76.47% <0%> (-1.48%) ⬇️

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 4d08138...1bd5a02. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Oct 6, 2017 •
edited
Loading

Codecov Report

Merging #2215 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2215      +/-   ##
==========================================
+ Coverage   72.31%   72.32%   +<.01%     
==========================================
  Files        1179     1179              
  Lines       58821    58832      +11     
  Branches     8465     8466       +1     
==========================================
+ Hits        42535    42548      +13     
+ Misses      14910    14908       -2     
  Partials     1376     1376
Flag Coverage Δ
#smoketests 72.32% <100%> (ø) ⬆️
#unittests 69.88% <100%> (ø) ⬆️
Impacted Files Coverage Δ
...faces/freesurfer/tests/test_auto_ConcatenateLTA.py 85.71% <ø> (ø) ⬆️
nipype/interfaces/freesurfer/preprocess.py 65.64% <100%> (+0.37%) ⬆️
nipype/pipeline/plugins/multiproc.py 78.16% <0%> (+1.4%) ⬆️

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 2b10ff2...1bcf9d1. Read the comment docs.

@effigies
Copy link
Member Author

effigies commented Oct 6, 2017

A quick review of this one would be greatly appreciated.

satra
satra reviewed Oct 6, 2017
View reviewed changes
nipype/interfaces/freesurfer/preprocess.py
if name == 'out_type':
value = {'VOX2VOX': 0, 'RAS2RAS': 1}[value]
return super(ConcatenateLTA, self)._format_arg(name, spec, value)

def _list_outputs(self):
Copy link
Member

Choose a reason for hiding this comment

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

perhaps this can be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm guessing something else would be needed to change? Removing _list_outputs gets me with an <undefined> outputs.out_file.

Copy link
Member

Choose a reason for hiding this comment

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

name_source in outputspec

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not able to figure out how to use name_source. Has no effect I can see in output spec, and trying to follow the examples in the docs results in failing to generate a string for inputs.out_file and the interface fails.

Copy link
Member

Choose a reason for hiding this comment

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

nevermind - the logic i had in mind is only implemented if name_source is in the input trait. i think the next refactor needs to take care of filename generation

@satra
Copy link
Member

satra commented Oct 6, 2017

@effigies - just merged oscar's terminal output PR, so make specs would need to be redone.

@effigies effigies force-pushed the enh/concatenate_lta branch from 1bd5a02 to 1bcf9d1 Compare October 6, 2017 13:39
@effigies
Copy link
Member Author

effigies commented Oct 6, 2017

@satra Any other comments? Good to merge?

@satra satra merged commit a79780a into nipy:master Oct 6, 2017
@effigies effigies deleted the enh/concatenate_lta branch October 19, 2017 16:51
@satra satra added this to the 0.14.0 milestone Oct 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@satra satra satra left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.14.0
Development

Successfully merging this pull request may close these issues.
3 participants
@effigies @codecov-io @satra