Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

handle unavailable traits due to version differences #3273

Merged
merged 6 commits into from Nov 23, 2020

Conversation

satra
Copy link
Member

@satra satra commented Nov 21, 2020

Summary

This turns all unavailable traits to undefined at runtime, and switches the order of checks. i'm not sure run is the best place for this yet.

Needs tests. @salma1601 and @effigies let's use this to see if we can address this.

Fixes #3138 .

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

  • Make unavailable traits undefined

Acknowledgment

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

Potential hack for #3138
@codecov
Copy link

codecov bot commented Nov 21, 2020

Codecov Report

Merging #3273 (35ea635) into master (7d538fa) will decrease coverage by 0.29%.
The diff coverage is 92.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3273      +/-   ##
==========================================
- Coverage   64.98%   64.69%   -0.30%     
==========================================
  Files         302      302              
  Lines       39947    39837     -110     
  Branches     5283     5286       +3     
==========================================
- Hits        25961    25773     -188     
- Misses      12910    12974      +64     
- Partials     1076     1090      +14     
Flag Coverage Δ
unittests 64.69% <92.30%> (-0.22%) ⬇️

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

Impacted Files Coverage Δ
nipype/interfaces/base/core.py 84.14% <92.30%> (+0.28%) ⬆️
nipype/testing/utils.py 70.90% <0.00%> (-18.19%) ⬇️
nipype/info.py 80.00% <0.00%> (-12.31%) ⬇️
nipype/interfaces/freesurfer/base.py 63.55% <0.00%> (-12.25%) ⬇️
nipype/workflows/__init__.py 88.23% <0.00%> (-11.77%) ⬇️
nipype/pkg_info.py 75.00% <0.00%> (-6.25%) ⬇️
nipype/scripts/cli.py 42.16% <0.00%> (-5.09%) ⬇️
nipype/interfaces/fsl/base.py 76.40% <0.00%> (-4.45%) ⬇️
nipype/interfaces/afni/base.py 65.54% <0.00%> (-3.99%) ⬇️
nipype/interfaces/diffusion_toolkit/base.py 46.15% <0.00%> (-3.85%) ⬇️
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d538fa...35ea635. Read the comment docs.

@effigies
Copy link
Member

Ah, thanks for the concrete example. I would be inclined to do it at __init__(), prior to setting inputs to kwargs, so that we don't silently hide things people try to set.

What about 2a7452e?

@effigies effigies added this to the 1.6.0 milestone Nov 21, 2020
@satra
Copy link
Member Author

satra commented Nov 21, 2020

I would be inclined to do it at __init__()

that would solve the issue of use_default and no additional input, but would still be silent and not catch things if people set those values anyway in code (either init or run or inputs). this would bomb downstream. so we may want to raise exceptions in run for unavailable_traits that are set in run as well. so both places?

@effigies
Copy link
Member

effigies commented Nov 21, 2020

We should be raising exceptions at runtime if they are set by the user:

self.inputs.trait_set(**inputs)
self._check_mandatory_inputs()
self._check_version_requirements(self.inputs)

def _check_version_requirements(self, trait_object, raise_exception=True):

if raise_exception:
raise Exception(
"Trait %s (%s) (version %s < required %s)"
% (name, self.__class__.__name__, version, min_ver)
)

I think we need to do the first one with raise_exception=False. Pushed e03466f...

@satra
Copy link
Member Author

satra commented Nov 21, 2020

@effigies - sounds good.

obj.inputs.foo = 1
obj.inputs.bar = 1
with caplog.at_level(logging.WARNING, logger="nipype.interface"):
obj._check_version_requirements(obj.inputs)
assert len(caplog.records) == 2
assert len(caplog.records) == 4
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changed the expected warnings... I'm not sure if we want them at init?

Copy link
Member Author

@satra satra Nov 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could suppress them in init.

@effigies
Copy link
Member

This is ready for review and merge, IMO.

@satra satra merged commit 3ec5eeb into master Nov 23, 2020
@effigies effigies deleted the fix-unavailable-traits branch November 23, 2020 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

N4BiasFieldCorrection fails for compiled ANTS
2 participants