Skip to content

ENH: Modify Directory and File traits to get along with pathlib #2959

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

Closed
wants to merge 19 commits into from

Conversation

oesteban
Copy link
Contributor

@oesteban oesteban commented Jul 16, 2019

Ref #2944

Summary

Fixes # .

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

Acknowledgment

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

@satra
Copy link
Member

satra commented Jul 16, 2019

looks like a reasonable PR. the File trait should take hash_files into account.

the Directory trait is a weird one since hash_files doesn't apply to it as we don't hash the contents of a directory.

@codecov-io
Copy link

codecov-io commented Jul 16, 2019

Codecov Report

Merging #2959 into master will decrease coverage by 0.03%.
The diff coverage is 78.22%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2959      +/-   ##
=========================================
- Coverage   64.24%   64.2%   -0.04%     
=========================================
  Files         342     342              
  Lines       43692   43727      +35     
  Branches     5453    5465      +12     
=========================================
+ Hits        28068   28074       +6     
- Misses      14545   14562      +17     
- Partials     1079    1091      +12
Flag Coverage Δ
#unittests 64.2% <78.22%> (-0.04%) ⬇️
Impacted Files Coverage Δ
nipype/info.py 88.05% <ø> (ø) ⬆️
nipype/interfaces/afni/base.py 66.66% <ø> (-0.27%) ⬇️
nipype/interfaces/base/support.py 79.65% <ø> (ø) ⬆️
nipype/interfaces/io.py 49.52% <0%> (-0.12%) ⬇️
nipype/interfaces/base/specs.py 87.24% <100%> (+0.54%) ⬆️
nipype/interfaces/dipy/base.py 77.96% <100%> (+0.77%) ⬆️
nipype/interfaces/spm/base.py 57.94% <100%> (-0.14%) ⬇️
nipype/interfaces/afni/preprocess.py 82.1% <100%> (+0.08%) ⬆️
nipype/interfaces/base/traits_extension.py 84.35% <80.68%> (-10.22%) ⬇️
nipype/pipeline/plugins/legacymultiproc.py 63% <0%> (-4.5%) ⬇️
... and 4 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 e722d61...8069604. Read the comment docs.

@satra
Copy link
Member

satra commented Jul 16, 2019

@oesteban - do we really need to enable usedefault=True for all File traits?

@oesteban
Copy link
Contributor Author

I'm thinking it should be enabled only if the default value is given.

@effigies
Copy link
Member

@oesteban That's counter to how all of the other traits work, which require manual enabling of usedefault=True. While it's a little annoying to have to add when you set a default, this change breaks consistency.

@oesteban
Copy link
Contributor Author

The current implementation of interfaces setting a default value on Files behave as if they had set usedefault=True.

@effigies
Copy link
Member

Oh, fun.

@satra
Copy link
Member

satra commented Jul 16, 2019

The current implementation of interfaces setting a default value on Files behave as if they had set usedefault=True.

i don't think this is true.

In [1]: from nipype.interfaces.base import TraitedSpec, File

In [2]: class A(TraitedSpec):
   ...:     foo = File()

In [3]: a = A()

In [4]: a
Out[4]: 

foo = <undefined>

In [5]: class A(TraitedSpec):
   ...:     foo = File('/foo')

In [6]: a = A()

In [7]: a
Out[7]: 

foo = <undefined>

In [12]: class A(TraitedSpec):
    ...:     foo = File(value='/foo')

In [13]: a = A()

In [14]: a
Out[14]: 

foo = <undefined>

and when usedefault=True then:

In [15]: class A(TraitedSpec):
    ...:     foo = File(value='/foo', usedefault=True)

In [16]: a = A()

In [17]: a
Out[17]: 

foo = /foo

@oesteban
Copy link
Contributor Author

Okay, yes, I double-checked and realized I was misinterpreting one test. Reverting.

oesteban added 3 commits July 16, 2019 10:01
dipy==0.15 apparently breaks almost all tests
dipy==0.14 breaks only one test (added skip condition)
dipy==0.16 (latest today) seems to work alright.
@oesteban oesteban marked this pull request as ready for review July 16, 2019 17:53
@effigies
Copy link
Member

It's pretty unclear what are the necessary changes and which ones were done while you were at it. If the new trait is a drop-in replacement, then all the SPM/AFNI changes are superfluous. If not, then are we breaking downstream code?

@oesteban
Copy link
Contributor Author

Closed in favor of #2962

@oesteban oesteban closed this Jul 16, 2019
@oesteban oesteban deleted the issue/2944-traits branch July 17, 2019 18:40
oesteban added a commit to oesteban/nipype that referenced this pull request Jul 31, 2019
oesteban added a commit to oesteban/nipype that referenced this pull request Jul 31, 2019
oesteban added a commit to oesteban/nipype that referenced this pull request Aug 1, 2019
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.

4 participants