Skip to content

MAINT: Simplify interface execution and better error handling of Node #3349

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

Merged
merged 10 commits into from
Sep 29, 2021

Conversation

oesteban
Copy link
Contributor

Revises the implementation of Node simplifying some details and improves the error handling of plugins. Introduces a NodeExecutionError (custom alias of RuntimeError) that allows more specificity with errors, if needed.

Requires: #3347.

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.

@codecov
Copy link

codecov bot commented Jun 24, 2021

Codecov Report

Merging #3349 (c0f49c7) into master (bb125ab) will increase coverage by 0.89%.
The diff coverage is 81.39%.

❗ Current head c0f49c7 differs from pull request most recent head bbcd4ff. Consider uploading reports for the commit bbcd4ff to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3349      +/-   ##
==========================================
+ Coverage   64.23%   65.13%   +0.89%     
==========================================
  Files         307      307              
  Lines       40373    40364       -9     
  Branches     5326     5333       +7     
==========================================
+ Hits        25935    26290     +355     
+ Misses      13385    13005     -380     
- Partials     1053     1069      +16     
Flag Coverage Δ
unittests 64.85% <81.39%> (?)

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

Impacted Files Coverage Δ
nipype/pipeline/plugins/tools.py 76.00% <ø> (-0.32%) ⬇️
nipype/pipeline/plugins/base.py 58.15% <50.00%> (-0.02%) ⬇️
nipype/interfaces/matlab.py 78.02% <66.66%> (+4.02%) ⬆️
nipype/pipeline/engine/nodes.py 78.93% <84.21%> (+1.56%) ⬆️
nipype/interfaces/base/core.py 87.95% <100.00%> (-0.23%) ⬇️
nipype/interfaces/base/support.py 82.19% <100.00%> (+1.26%) ⬆️
nipype/pipeline/plugins/linear.py 90.69% <100.00%> (+12.12%) ⬆️
nipype/interfaces/ants/registration.py 73.09% <0.00%> (ø)
nipype/interfaces/spm/preprocess.py 50.44% <0.00%> (+0.88%) ⬆️
... and 19 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 bb125ab...bbcd4ff. Read the comment docs.

@effigies
Copy link
Member

@oesteban Rebase to clean up the diff?

@effigies effigies force-pushed the enh/robuster-node branch from 104c3cc to 5210a3b Compare July 29, 2021 17:24
@effigies
Copy link
Member

@oesteban Rebased on your behalf.

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.

Overall I think it looks good. A couple initial thoughts, and please bug me to do another review.

Co-authored-by: Chris Markiewicz <effigies@gmail.com>
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. Suggested updating the linear to have the same exception behavior as distributed, but don't feel very strongly.

oesteban and others added 2 commits September 29, 2021 10:08
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
@effigies
Copy link
Member

Failing test fixed in #3378. Merging.

@effigies effigies merged commit e3a1192 into nipy:master Sep 29, 2021
@oesteban oesteban deleted the enh/robuster-node branch September 29, 2021 21:49
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