Skip to content

vectorization improvements for TPS transform(), apply(), and inverse()#129

Merged
fcollman merged 5 commits intoAllenInstitute:developfrom
djkapner:vectorize_TPS_apply
Dec 17, 2018
Merged

vectorization improvements for TPS transform(), apply(), and inverse()#129
fcollman merged 5 commits intoAllenInstitute:developfrom
djkapner:vectorize_TPS_apply

Conversation

@djkapner
Copy link
Copy Markdown
Contributor

@djkapner djkapner commented Nov 29, 2018

No description provided.

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 29, 2018 •
edited
Loading

Codecov Report

Merging #129 into develop will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #129   +/-   ##
========================================
  Coverage    95.65%   95.65%           
========================================
  Files           29       29           
  Lines         2000     2000           
========================================
  Hits          1913     1913           
  Misses          87       87
Impacted Files Coverage Δ
renderapi/transform/leaf/thin_plate_spline.py 100% <100%> (ø) ⬆️
renderapi/stack.py 95.33% <0%> (+0.12%) ⬆️
renderapi/transform/leaf/polynomial_models.py 93.17% <0%> (+0.17%) ⬆️

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 c14d5e0...21b0185. Read the comment docs.

fcollman
fcollman reviewed Nov 29, 2018
View reviewed changes
renderapi/transform/leaf/thin_plate_spline.py Outdated
from renderapi.utils import encodeBase64, decodeBase64
from .transform import Transform
import scipy.spatial
import scipy.special
Copy link
Copy Markdown
Member

@fcollman fcollman Nov 29, 2018

Choose a reason for hiding this comment

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

duplicate line

Copy link
Copy Markdown
Contributor Author

@djkapner djkapner Nov 29, 2018

Choose a reason for hiding this comment

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

it is not duplicate, but I ended up not using scipy.special ... would have thought flake8 would have told me that... will remove

@djkapner
Copy link
Copy Markdown
Contributor Author

djkapner commented Nov 30, 2018 •
edited
Loading

let's not merge this just yet. I found a scipy thin plate radial basis function that is even faster...
scratch that... I did some more testing, it wasn't faster.

@djkapner djkapner mentioned this pull request Dec 14, 2018
Merged
@fcollman
Copy link
Copy Markdown
Member

fcollman commented Dec 17, 2018

@RussTorres i'm good with merging this are you?

@RussTorres
Copy link
Copy Markdown
Collaborator

RussTorres commented Dec 17, 2018

I mentioned this before, but not as a review -- this PR makes scipy an actual dependency. If we're okay with that (and I think we are) we should add it to the requirements file.

@djkapner
Copy link
Copy Markdown
Contributor Author

djkapner commented Dec 17, 2018

I will make that change in the PR once Forrest confirms he is ok with the dependency

@djkapner
Copy link
Copy Markdown
Contributor Author

djkapner commented Dec 17, 2018

there do appear to be a couple of numpy only ideas instead of scipy.spatial.cdist:
https://stackoverflow.com/questions/52030458/vectorized-spatial-distance-in-python-using-numpy

@fcollman
Copy link
Copy Markdown
Member

fcollman commented Dec 17, 2018

scipy should be moved from test_requirements to requirements.

@fcollman fcollman merged commit 05719f1 into AllenInstitute:develop Dec 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fcollman fcollman fcollman left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@djkapner @fcollman @RussTorres