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.

Sign up for GitHub

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

Jump to bottom

ENH: Base implementation of B-Spline transforms #138

Merged
merged 11 commits into from
Feb 24, 2022
Merged

ENH: Base implementation of B-Spline transforms #138

merged 11 commits into from
Feb 24, 2022

Conversation

oesteban
Copy link
Collaborator

Addressing #106.

@oesteban oesteban force-pushed the enh/bring-bspline branch 3 times, most recently from 41a3505 to 3ad562e Compare February 10, 2022 12:46
@pep8speaks
Copy link

pep8speaks commented Feb 23, 2022 •
edited
Loading

Hello @oesteban! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-02-24 11:04:52 UTC

@oesteban oesteban mentioned this pull request Feb 23, 2022
Open
@oesteban oesteban marked this pull request as ready for review February 23, 2022 16:38
@oesteban
Copy link
Collaborator Author

I think this base chunk of code can be reviewed. Please note I'm aware of #154.

@codecov-commenter
Copy link

codecov-commenter commented Feb 23, 2022 •
edited
Loading

Codecov Report

Merging #138 (f232acf) into master (f703445) will increase coverage by 0.06%.
The diff coverage is 98.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #138      +/-   ##
==========================================
+ Coverage   97.82%   97.88%   +0.06%     
==========================================
  Files          12       13       +1     
  Lines        1104     1184      +80     
  Branches      171      183      +12     
==========================================
+ Hits         1080     1159      +79     
  Misses         18       18              
- Partials        6        7       +1
Flag Coverage Δ
travis 97.80% <98.83%> (+0.15%) ⬆️
unittests 97.81% <98.83%> (+0.07%) ⬆️

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

Impacted Files Coverage Δ
nitransforms/nonlinear.py 98.76% <98.30%> (-1.24%) ⬇️
nitransforms/base.py 100.00% <100.00%> (ø)
nitransforms/interp/bspline.py 100.00% <100.00%> (ø)

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 f703445...f232acf. Read the comment docs.

effigies
effigies reviewed Feb 23, 2022
View reviewed changes
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.

Note to self: pick back up at to_field().

nitransforms/interp/bspline.py Show resolved Hide resolved
nitransforms/interp/bspline.py Outdated Show resolved Hide resolved
nitransforms/nonlinear.py Outdated Show resolved Hide resolved
nitransforms/nonlinear.py Outdated Show resolved Hide resolved
nitransforms/nonlinear.py
Comment on lines +45 to +48
field = _ensure_image(field)
self._field = np.squeeze(
np.asanyarray(field.dataobj) if hasattr(field, "dataobj") else field
)
Copy link
Member

Choose a reason for hiding this comment

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

No need to change anything, but I think _ensure_image doesn't quite capture the operation of something that will pass ndarrays through untouched.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this a comment on the function's name, the function's behavior, or both?
Happy to move towards something more appropriate.

nitransforms/nonlinear.py Outdated Show resolved Hide resolved
nitransforms/nonlinear.py Outdated Show resolved Hide resolved
effigies
effigies reviewed Feb 23, 2022
View reviewed changes
nitransforms/nonlinear.py Show resolved Hide resolved
nitransforms/interp/bspline.py Outdated Show resolved Hide resolved
effigies
effigies approved these changes Feb 23, 2022
View reviewed changes
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.

Based on your comment, I didn't really try to understand _map_xyz. The remaining parts seem sensible. Some final comments.

nitransforms/nonlinear.py Show resolved Hide resolved
nitransforms/nonlinear.py Outdated Show resolved Hide resolved
nitransforms/nonlinear.py Outdated Show resolved Hide resolved
nitransforms/nonlinear.py Outdated Show resolved Hide resolved
oesteban and others added 4 commits February 23, 2022 21:36
@oesteban @effigies
Apply suggestions from code review
997cbac 
Co-authored-by: Chris Markiewicz <markiewicz@stanford.edu>
@oesteban @effigies
fix: address review comments
4eca2fb 
Co-authored-by: Chris Markiewicz <markiewicz@stanford.edu>
@oesteban
fix: final amends to ensure tests pass
07ade2b
@oesteban
fix: fixed error types and increased test coverage
f232acf
@oesteban oesteban merged commit 6fbcbc1 into master Feb 24, 2022
@oesteban oesteban deleted the enh/bring-bspline branch February 24, 2022 13:34
@oesteban oesteban linked an issue Feb 25, 2022 that may be closed by this pull request
Bring B-Spline interpolation code from SDCFlows #106
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@effigies effigies effigies approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bring B-Spline interpolation code from SDCFlows
4 participants
@oesteban @pep8speaks @codecov-commenter @effigies