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: do integer**2. inplace #8896

Merged
merged 1 commit into from
Apr 9, 2017
Merged

Conversation

juliantaylor
Copy link
Contributor

Squaring integer arrays with float argument always casts to double so
the squaring itself can be done inplace.

@juliantaylor juliantaylor force-pushed the inplace-square branch 2 times, most recently from 02cbcc4 to 34c7c4d Compare April 5, 2017 12:49
Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Looks good, modulo the one small comment.

@@ -589,24 +589,24 @@ fast_scalar_power(PyArrayObject *a1, PyObject *o2, int inplace)
(a1, (PyObject *)a1, fastop);
}
else {
PyArray_Descr *dtype = NULL;
PyObject *res;
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't need res any more.

@mhvk
Copy link
Contributor

mhvk commented Apr 6, 2017

Actually, a bigger comment after all: why is a**2. replaced by np.multiply(a, a) rather than just by np.square(a)? That should be even faster (of course, that also holds in the float-chain above).

@juliantaylor
Copy link
Contributor Author

juliantaylor commented Apr 6, 2017

good question, square should work just fine.
It doesn't make a difference for the float path as multiply is special cased for both inputs being the same already. But it should have a small effect on the integer paths.
Updated to use square and fixed a refcount leak.

@mhvk
Copy link
Contributor

mhvk commented Apr 6, 2017

All looks good to me. Any reason not to merge?

assert_((x**2.00001).dtype is (x**2.0).dtype)
res = x**2.0
assert_((x**2.00001).dtype is res.dtype)
assert_(res is not x)
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd like a not np.shares_memory(res, x) here too - far too easy to break identity while still having a view.

Also a check that x is still [1, 2, 3] would do the job

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, done

Squaring integer arrays with float argument always casts to double so
the squaring itself can be done inplace.
Also use square fastop instead of multiply.
Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@eric-wieser
Copy link
Member

Release note?

@juliantaylor
Copy link
Contributor Author

this is really minor, not worth a note

@eric-wieser
Copy link
Member

Was thinking it would fit in with the other comments about avoiding temporaries, but fair enough.

@juliantaylor juliantaylor merged commit f8d4a2e into numpy:master Apr 9, 2017
@juliantaylor juliantaylor deleted the inplace-square branch April 9, 2017 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants