nipype

nipype copied to clipboard

Summary

Addresses #3138 in the short term.

List of changes proposed in this PR (pull-request)

removed use_default=True for rescale_intensities input

Acknowledgment

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

Jan 04 '20 20:01 salma1601

Hi @salma1601, ~~why is this a bug?~~

EDIT: sorry, I hit send to fast - somehow I failed to read the issue you linked.

Jan 07 '20 07:01 oesteban

@effigies and @oesteban - see the discussion in #3138 - there is a more general problem for which this PR is not the answer.

Jan 07 '20 14:01 satra

@satra I saw that. Is your preference to solve the more general problem instead of this short-term solution, or in addition?

Jan 07 '20 14:01 effigies

the short term solution makes an interface inputs change for ants versions greater than min_ver. this could potential change any scripts that are relying on the default. in this specific instance it may be fine if that is also the default in ANTs for versions > min_ver and that this change would in that case not affect the outcome. it will force things to rerun (since the hash will change). i would be fine with this PR, if we can confirm that the lack of the input on the command line will not make any difference to the outcome for ANTs for versions > min_ver

ps. i think we should discuss the more general solution in a more general discussion about how to version inputs and package versions. as the interfaces start evolving a bit, we will be left with what dependencies they support. this is similar in concept to any library api (e.g., numpy), but thus far we have stayed out of it by doing minimal pieces. as we start moving interfaces to their own packages for nipype 2, it may help to discuss the mechanics of this a bit.

Jan 07 '20 14:01 satra

i would be fine with this PR, if we can confirm that the lack of the input on the command line will not make any difference to the outcome for ANTs for versions > min_ver

@salma1601 could you confirm this with ANTs devs?

ps. i think we should discuss the more general solution in a more general discussion about how to version inputs and package versions. as the interfaces start evolving a bit, we will be left with what dependencies they support. this is similar in concept to any library api (e.g., numpy), but thus far we have stayed out of it by doing minimal pieces. as we start moving interfaces to their own packages for nipype 2, it may help to discuss the mechanics of this a bit.

👍

Jan 07 '20 16:01 oesteban

@oesteban @satra Unfortunately the ANTS default (at least for my version compiled 22 december 2019) is rescale_intensities=True, so actually it will make a difference ...

Jan 08 '20 08:01 salma1601

Is there consensus on whether a short-term fix is possible, or only the long-term approach discussed in https://github.com/nipy/nipype/issues/3138#issuecomment-571394934?

Feb 23 '20 16:02 effigies

Just checking back in on this in the run-up to another release. Apologies for the short notice...

Aug 15 '20 15:08 effigies

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 67.62%. Comparing base (f277d18) to head (5851a63).

:exclamation: Current head 5851a63 differs from pull request most recent head f3a33f4. Consider uploading reports for the commit f3a33f4 to get more accurate results

@@            Coverage Diff             @@
##           master    #3139      +/-   ##
==========================================
+ Coverage   63.15%   67.62%   +4.47%     
==========================================
  Files         308      299       -9     
  Lines       40825    39480    -1345     
  Branches     5656     5220     -436     
==========================================
+ Hits        25781    26698     +917     
+ Misses      14031    12070    -1961     
+ Partials     1013      712     -301     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Mar 16 '24 12:03 codecov[bot]