FIX: Un-set incorrect default options in TOPUP #2637
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2637 +/- ##
=========================================
- Coverage 67.62% 64.02% -3.6%
=========================================
Files 340 338 -2
Lines 43003 42952 -51
Branches 5321 5318 -3
=========================================
- Hits 29080 27501 -1579
- Misses 13225 14394 +1169
- Partials 698 1057 +359
Continue to review full report at Codecov.
|
nipype/interfaces/fsl/epi.py
Outdated
| '.')) | ||
| subsamp = traits.Int(1, usedefault=True, | ||
| argstr='--subsamp=%d', desc='sub-sampling scheme') | ||
| '(default in FSL 10).')) |
There was a problem hiding this comment.
what are FSL 10, FSL 1, and FSL 8?
There was a problem hiding this comment.
when I'm reading this today it's not clear indeed... I meant that in (a version of) FSL the default value for this arg is 10.
The story is that some months ago I added usedefault=True to places where default value of traits was chosen, but this changed the output of some workflows and we got complains about it, so I thought that we can just keep the FSL defaults and only inform about these values (but in the description, not in the trait's default). I'm open to other suggestions, I'm really not sure what is the best strategy.
There was a problem hiding this comment.
Isn't the problem that the documented defaults and actual defaults differ, and thus passing these "default" values changes the behavior of the interface? If so, I think adding this description will confuse users.
I think I would, for now, make a comment in the interface docstring such as:
Some default values provided in the TOPUP documentation appear to differ from
the defaults used when no argument is provided.
For reference we provide the following list.
If an empirical default value is known, it is accurate as of version <VERSION>.
====== =========== =========
Option Documented Empirical
====== =========== =========
``warp_res`` 10 <UNKNOWN>
``othervar`` 9 8
...
====== =========== =========There was a problem hiding this comment.
@effigies - yes this is probably the main problem, some flags shouldn't be used at all as default. I will review this later and add the table you suggested. It is extra work for us to keep this updated, but it might be the only solution that make sense...
This is not the only interface that might have similar problems (at least not the only one that possibly was changed after my PR with usedefault=True)
There was a problem hiding this comment.
Maybe put a big warning that the values may be out-of-date? We're not going to be able to regularly update it, but letting people know there's an issue worth looking into and what we knew at this point seems like the best we can do.
|
@effigies @satra - I removed the misleading info about the defaults values. I was planning to do more, but I gave up, wasn't sure how to provide more information without possible confusion. Two more problems, that I can always address in the future PR:
|
effigies
changed the title
|
@djarecka Good to merge? (I'm ignoring the Travis failures. Should be fixed when merged into master.) |
|
Ah, see you removed WIP. Merging. |
This is follow-up to the #2533. I started from
topup, issues were reported by @atsuch in the comments to the PR.usedefault=Trueand introduced default value in the description (it's probably better to let FSL set these values)If this is ok, I should probably add similar changes to other FSL interfaces changed in #2533