MAINT: Various minor improvements to complement previous PR #2964
Conversation
| @@ -22,7 +22,10 @@ def test_ACompCor_inputs(): | |||
| ), | |||
| num_components=dict(xor=['variance_threshold'], ), | |||
| pre_filter=dict(usedefault=True, ), | |||
| realigned_file=dict(mandatory=True, ), | |||
| realigned_file=dict( | |||
| extensions=None, | |||
There was a problem hiding this comment.
In the spec tests, only explicitly specified metadata has gone in, previously. Why is extensions different?
There was a problem hiding this comment.
This is an oversight from the previous PR - 6c3858a#diff-07934241447dd5e65f51347e36fdd1ffR291
There was a problem hiding this comment.
Right. But, for example, exists=False is not in all of these, even though that's the default. Why is extensions=<default> in here? Is this something we can fix in the trait, instead?
There was a problem hiding this comment.
We could (and it was like that before that commit) replicate the same behavior of exists. Namely, the metadata is not propagated up to the base classes. I'm clueless about the implications of this.
For exists I just kept the original behavior.
There was a problem hiding this comment.
i think the intent was not so much to track the defaults of the class, but keywords that the user put in explicitly.
i do agree that these underlying metadata (if changed internally in traits_extension) will change the behavior of interfaces and it would be good to catch them. another side note, is how we deal with and improve pickled crashfile compatibility.
if we keep old behavior, then we must commit to not changing underlying defaults in traits_extension in the future.
There was a problem hiding this comment.
@satra Just to clarify, adding extensions metadata to everything is preferable?
There was a problem hiding this comment.
@effigies - if we do want to take note of underlying metadata changes, we either write a set of tests for each trait type that's used in nipype, and not add the defaults to these auto-tests.
or we simply pull every default into the auto test. more generally though, my hope is that we can move closer to the kind of testing we are planning to do with the afni folks. also start splitting interfaces into their own packages.
so for now, i think this ok. but we should think this through/generalize our goals a bit better.
Co-Authored-By: Chris Markiewicz <effigies@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #2964 +/- ##
==========================================
- Coverage 67.6% 63.89% -3.72%
==========================================
Files 344 342 -2
Lines 43797 43741 -56
Branches 5471 5468 -3
==========================================
- Hits 29609 27947 -1662
- Misses 13478 14678 +1200
- Partials 710 1116 +406
Continue to review full report at Codecov.
|
|
Tests from 0052814 are all green. 👍 This is ready to go. |
|
I'll merge this before diving into addressing #2944 |
In nipy#2964 I broke some interfaces (@tsalo identified `Allineate` as one of those) by removing the ``_get_fname`` function from the base ``AFNICommand``. The intent was to migrate to a ``name_source``-based management of automatically generated names, but in the case of ``Allineate``, the use of `_get_fname` was a bit different (modifying the user-provide input value under certain conditions to follow ANFI's ``3dAllineate`` behavior). This PR restores the missing function. Closes nipy#3070.
In nipy#2964 I broke some interfaces (@tsalo identified `Allineate` as one of those) by removing the ``_get_fname`` function from the base ``AFNICommand``. The intent was to migrate to a ``name_source``-based management of automatically generated names, but in the case of ``Allineate``, the use of `_get_fname` was a bit different (modifying the user-provide input value under certain conditions to follow ANFI's ``3dAllineate`` behavior). Also remove a definition of the same function for ``Deconvolve``. This PR restores the missing function. Closes nipy#3070.
Complements #2962 with comments from @mgxd that were left out and some scraps from #2959.