FIX Ants N4BiasFieldCorrection rescale_intensities bug

[salma1601]

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

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

[oesteban]

Hi @salma1601, why is this a bug?

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

[oesteban]

Okay, this looks like an effective way of fixing the bug. Have you run black on the code before committing?

[effigies]

There was a move a while back to either remove defaults if unused, or set usedefault=True if the default was specified. In this case, if we're removing usedefault, we should also remove the default.

Suggested change

False,

[satra]

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

[effigies]

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

[satra]

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.

[oesteban]

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.

👍

[salma1601]

@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 ...

[effigies]

Is there consensus on whether a short-term fix is possible, or only the long-term approach discussed in #3138 (comment)?

[effigies]

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

Additional details and impacted files

@@            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     

Flag	Coverage Δ
smoketests	53.02% <ø> (?)
unittests	64.79% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.