changing ANTS input traits, fixes #2245 #2261
Conversation
Codecov Report
@@ Coverage Diff @@ ## master #2261 +/- ## ========================================== - Coverage 72.37% 72.32% -0.06% ========================================== Files 1188 1183 -5 Lines 59184 59008 -176 Branches 8506 8490 -16 ========================================== - Hits 42837 42675 -162 + Misses 14961 14949 -12 + Partials 1386 1384 -2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@ ## master #2261 +/- ## ========================================== - Coverage 72.37% 72.32% -0.06% ========================================== Files 1188 1183 -5 Lines 59184 59008 -176 Branches 8506 8490 -16 ========================================== - Hits 42837 42675 -162 + Misses 14961 14949 -12 + Partials 1386 1384 -2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@ ## master #2261 +/- ## ========================================== - Coverage 72.37% 72.32% -0.06% ========================================== Files 1188 1184 -4 Lines 59184 59024 -160 Branches 8506 8490 -16 ========================================== - Hits 42837 42689 -148 + Misses 14961 14953 -8 + Partials 1386 1382 -4
Continue to review full report at Codecov.
|
| @djarecka - can we use this example to add a test to regression testing framework? i feel that's what we need to do from here on out. interfaces like antsRegistration have too many options to generate all kinds of tests automatically. but we can use a combination of manual examples that actually test the binary. this obviously won't be part of the pull-request, but it would be nice to use this example as a test case in the testing framework. |
| desc='the metric weight(s) for each stage. ' | ||
| 'The weights must sum to 1 per stage.') | ||
| | ||
| radius = traits.List(traits.Int(), requires=['metric'], mandatory=True, desc='') |
There was a problem hiding this comment.
I added some description, but originally none of the traits had any description, and most of the still don't have. I can write the description when working on tests. I guess we can keep this PR open for some time
There is any typical value that could be used as default?
| @satra : ok, will work on that |
| @djarecka - a mandatory trait should not have a default value in general. we leave it to the user to provide it. but there are cases when this may be necessary. i don't think this is one of them. descriptions should come from the actual command line interface. |
| @satra - hmm... i used default for |
| @djarecka - is this ready? or there other changes coming? |
| @satra it fixes the issue (and just added a simple example that I was using to test it. However this entire file requires review and tests. Other interfaces have very similar problems. We can either keep this open or open a new one later. If no |
| ants.inputs.transformation_model= "SyN" | ||
| ants.inputs.moving_image = [os.path.join(datadir, 'resting.nii')] | ||
| ants.inputs.fixed_image = [os.path.join(datadir, 'T1.nii')] | ||
| ants.inputs.metric = [u'MI'] |
There was a problem hiding this comment.
can we just import unicode literals?
| metric_weight = traits.List(traits.Float(), value=[1.0], usedefault=True, | ||
| requires=['metric'], mandatory=True, | ||
| desc='the metric weight(s) for each stage. ' | ||
| 'The weights must sum to 1 per stage.') |
There was a problem hiding this comment.
isn't this a pep8 warning?
There was a problem hiding this comment.
this line doesn't give any warning, but the files indeed violates many. will edit
There was a problem hiding this comment.
I will actually review pep8 only for the first part I was changing, will review pep8 for entire file together with other updates
| import os | ||
| import pytest | ||
| | ||
| def test_ants_mand(): |
There was a problem hiding this comment.
does the interface actually create files? if so (and I guess it would), we should include tmpdir.chdir()
5 participants
I've changed
metric_weight(took default from other interfaces) andradius(left without default value).Unfortunately other interfaces from that file have similar problems (do not work after providing all mandatory fields) and it was already reported in #529.
I can keep adding
mandatoryto traits and guessing default values, but it might be better for those interfaces if someone who is using them do it (or help me).