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

FIX: Use load_resultfile when loading a results pickle #2985

Merged
merged 2 commits into from
Aug 6, 2019

Conversation

oesteban
Copy link
Contributor

@oesteban oesteban commented Aug 2, 2019

Summary

Some sections of the code were using filemanip.utils.loadpkl, which does not resolve paths (probably dodging more frequent errors from modify_paths akin to #2944).

These changes are spun off of #2971.

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

  • A revision of loadpkl for readability and reliability.
  • Replacement of instances where loadpkl was used for reading result files instead of pipeline.engine.utils.load_resultfile.

Acknowledgment

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

Some sections of the code were using ``loadpkl`` which does not resolve
paths.

This PR also improves ``loadpkl`` for readability and reliability.
@codecov
Copy link

codecov bot commented Aug 2, 2019

Codecov Report

Merging #2985 into master will decrease coverage by 3.35%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2985      +/-   ##
==========================================
- Coverage    67.6%   64.24%   -3.36%     
==========================================
  Files         344      342       -2     
  Lines       43802    43741      -61     
  Branches     5478     5473       -5     
==========================================
- Hits        29611    28101    -1510     
- Misses      13475    14551    +1076     
- Partials      716     1089     +373
Flag Coverage Δ
#smoketests ?
#unittests 64.24% <80%> (-0.8%) ⬇️
Impacted Files Coverage Δ
nipype/pipeline/engine/nodes.py 77.31% <100%> (-6.79%) ⬇️
nipype/pipeline/engine/utils.py 69.62% <100%> (-10.31%) ⬇️
nipype/pipeline/plugins/base.py 59.12% <33.33%> (-1.2%) ⬇️
nipype/utils/filemanip.py 76.1% <80%> (-3.98%) ⬇️
nipype/interfaces/nilearn.py 40% <0%> (-56.67%) ⬇️
nipype/utils/spm_docs.py 25.92% <0%> (-44.45%) ⬇️
nipype/interfaces/freesurfer/base.py 50% <0%> (-30.51%) ⬇️
nipype/utils/logger.py 59.7% <0%> (-29.86%) ⬇️
nipype/algorithms/rapidart.py 35% <0%> (-29.42%) ⬇️
nipype/interfaces/spm/base.py 57.94% <0%> (-29.14%) ⬇️
... and 44 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 865adc9...88b6fc0. Read the comment docs.

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for breaking these fixes down into digestible PRs, and sorry for the delay.

One minor suggestion, but absolutely not a big deal. I can submit a separate PR.

nipype/utils/filemanip.py Show resolved Hide resolved
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.

2 participants