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

ENH: Avoid loading result from file when writing reports #3024

Merged
merged 4 commits into from Sep 11, 2019

Conversation

oesteban
Copy link
Contributor

Summary

Minimize the access to the result property when writing
pre/post-execution reports.

This modification should particularly preempt #3009 (comment)

Acknowledgment

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

@codecov
Copy link

codecov bot commented Sep 11, 2019

Codecov Report

Merging #3024 into master will decrease coverage by 3.38%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3024      +/-   ##
==========================================
- Coverage   67.52%   64.13%   -3.39%     
==========================================
  Files         344      342       -2     
  Lines       44045    43984      -61     
  Branches     5552     5548       -4     
==========================================
- Hits        29740    28211    -1529     
- Misses      13566    14646    +1080     
- Partials      739     1127     +388
Flag Coverage Δ
#smoketests ?
#unittests 64.13% <66.66%> (-0.84%) ⬇️
Impacted Files Coverage Δ
nipype/pipeline/engine/nodes.py 76.49% <100%> (-7.62%) ⬇️
nipype/pipeline/engine/utils.py 70.81% <62.5%> (-10.12%) ⬇️
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%) ⬇️
nipype/utils/provenance.py 55.73% <0%> (-28.35%) ⬇️
nipype/interfaces/fsl/model.py 55.26% <0%> (-25.35%) ⬇️
... 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 0470641...769b941. Read the comment docs.

@oesteban oesteban reopened this Sep 11, 2019
@oesteban
Copy link
Contributor Author

@effigies, if you could spare one minute with this I'd really appreciate it. Especially, the new additions to make Path more consistent across 3.x versions - https://github.com/nipy/nipype/pull/3024/files#diff-7d84fc0957a520f25bb02479beb87c51R75 (see also some tests that I added).

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.

This seems reasonable overall. A couple comments on the Path hacks. In addition, would it be possible to tag the regions that are compensating for failures in specific Python versions with # PY34, # PY35, etc? That will make them easier to find and purge when we bump our minimum Python.

nipype/utils/filemanip.py Outdated Show resolved Hide resolved
nipype/utils/tests/test_filemanip.py Outdated Show resolved Hide resolved
nipype/utils/filemanip.py Outdated Show resolved Hide resolved
oesteban and others added 3 commits September 11, 2019 10:57
Minimize the access to the ``result`` property when writing
pre/post-execution reports.

This modification should particularly preempt nipy#3009 (comment)
Co-Authored-By: Chris Markiewicz <effigies@gmail.com>
@oesteban oesteban force-pushed the enh/robuster-write-report branch 2 times, most recently from c7735db to c5d3128 Compare September 11, 2019 18:09
@effigies effigies added this to the 1.2.3 milestone Sep 11, 2019
@oesteban oesteban merged commit 4c414b8 into nipy:master Sep 11, 2019
@oesteban oesteban deleted the enh/robuster-write-report branch September 11, 2019 21:18
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.

None yet

3 participants