-
Notifications
You must be signed in to change notification settings - Fork 213
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 a fast-path for inplace transforms #1202
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1202 +/- ##
==========================================
+ Coverage 96.25% 96.28% +0.02%
==========================================
Files 20 20
Lines 1791 1805 +14
==========================================
+ Hits 1724 1738 +14
Misses 67 67
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@@ -789,6 +789,22 @@ def transform( # pylint: disable=invalid-name | |||
'33 98' | |||
|
|||
""" | |||
if inplace and getattr(xx, "typecode", "") == "d": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts about moving this to _copytobuffer
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could move it to the top of the function so it would skip everything else if true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also make DataType.ARRAY
the first type checked for in _convertback
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I tried that too and it unfortunately still adds quite a bit of overhead with both of those. 1.5 seconds vs 1 second on my laptop here.
Although, both of those would be "easy" immediate gains.
Another alternative to consider is to add a method called |
I like this idea! It could be a bit confusing that you'd need a buffer array to pass to it for the speed though? (i.e. |
I was thinking of adding
That is a good goal. I suggest addressing that issue separately. |
OK, I did some benchmarking on the single-point versions, and casting immediately to a single-value float array and using those in the generics is still a massive improvement. I'm sure more can be done by calling into a different Cython function too... see here: #1203 |
Rather than opening an issue, I figured it would be easier to push up a PR with code for discussion purposes. This is looking at micro-optimizations when repeatedly calling
Transformer.transform()
on a single point at a time (yes, it is way better to make a large array first, but unfortunately that isn't always feasible). When transforming small arrays, there is a very large overhead (~40%) associated with calling into_copytobuffer
/_convertback
on every input/output array.I'm wondering if anyone has any thoughts on making a fast-path for inplace arrays to bypass the utils functions? Right now the code is very friendly and if a user requests inplace, but the array doesn't work as input array it will gracefully transform it to an array that works, but this adds quite a few checks along the way that are inconsequential for large arrays, but make a difference for small arrays.
Essentially what I want is access to the private Cython inplace
_transform()
method, so perhaps another way to handle this would be to expose that in a public way somehow?Benchmarking code:
Then running:
history.rst
for all changes andapi/*.rst
for new API