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: Add affine_tolerance flag to MergeSeries #706

Merged
merged 2 commits into from
Apr 8, 2022

Conversation

mgxd
Copy link
Contributor

@mgxd mgxd commented Apr 8, 2022

This should help clear up the error found in nipreps/fmriprep#2706

@mgxd mgxd requested a review from effigies April 8, 2022 16:20
@pep8speaks
Copy link

pep8speaks commented Apr 8, 2022

Hello @mgxd, Thank you for updating!

Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally, pip install flake8 and then run flake8 niworkflows.

Comment last updated at 2022-04-08 19:50:29 UTC

@codecov-commenter
Copy link

codecov-commenter commented Apr 8, 2022

Codecov Report

Merging #706 (fd3bf89) into maint/1.4.x (6373632) will increase coverage by 3.23%.
The diff coverage is 80.21%.

❗ Current head fd3bf89 differs from pull request most recent head de1593c. Consider uploading reports for the commit de1593c to get more accurate results

@@               Coverage Diff               @@
##           maint/1.4.x     #706      +/-   ##
===============================================
+ Coverage        45.94%   49.18%   +3.23%     
===============================================
  Files               46       49       +3     
  Lines             5641     5866     +225     
  Branches           809      837      +28     
===============================================
+ Hits              2592     2885     +293     
+ Misses            2962     2872      -90     
- Partials            87      109      +22     
Flag Coverage Δ
reportlettests ∅ <ø> (∅)
unittests 49.18% <80.21%> (+3.23%) ⬆️

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

Impacted Files Coverage Δ
niworkflows/interfaces/bids.py 96.21% <ø> (ø)
niworkflows/interfaces/norm.py 25.86% <ø> (ø)
niworkflows/viz/plots.py 22.12% <67.88%> (+16.18%) ⬆️
niworkflows/interfaces/plotting.py 71.83% <73.68%> (-1.51%) ⬇️
niworkflows/interfaces/nibabel.py 70.96% <80.55%> (+4.30%) ⬆️
niworkflows/utils/timeseries.py 83.33% <83.33%> (ø)
niworkflows/utils/testing.py 86.88% <86.88%> (ø)
niworkflows/interfaces/morphology.py 100.00% <100.00%> (ø)
niworkflows/utils/bids.py 75.24% <100.00%> (+0.24%) ⬆️

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 6373632...de1593c. 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.

Looks fine. I'd make the default threshold explicit.

niworkflows/interfaces/nibabel.py Show resolved Hide resolved
niworkflows/interfaces/nibabel.py Show resolved Hide resolved
niworkflows/interfaces/nibabel.py Show resolved Hide resolved
@mgxd mgxd force-pushed the enh/merge-tolerance branch from 78e3f5b to fd3bf89 Compare April 8, 2022 19:13
@effigies
Copy link
Member

effigies commented Apr 8, 2022

@mgxd sorry to do this, but we're going to need this in maint/1.4.x to get it into fmriprep 21.0.x. Or do we just want to tell these users to upgrade to 22.0?

@mgxd mgxd changed the base branch from master to maint/1.4.x April 8, 2022 19:45
@mgxd mgxd force-pushed the enh/merge-tolerance branch from fd3bf89 to de1593c Compare April 8, 2022 19:50
@mgxd
Copy link
Contributor Author

mgxd commented Apr 8, 2022

Sounds good - I changed the PR to target 1.4.x

@effigies
Copy link
Member

effigies commented Apr 8, 2022

Thanks. It did pass, so I'm going to go ahead and merge.

@effigies effigies merged commit 1686030 into nipreps:maint/1.4.x Apr 8, 2022
@mgxd mgxd deleted the enh/merge-tolerance branch April 8, 2022 19:57
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