-
-
Notifications
You must be signed in to change notification settings - Fork 11.3k
BUG: make round consistently return a copy #29137
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
Conversation
Needs a release note. |
Release note snippet added. |
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.
Seems nobody had an opinion about this besides mild agreement, so let's get this in.
If you are excited, about test for that order fix would be nice. We don't tend to strictly guarantee it, but it's the better thing when it's easy.
@@ -637,8 +637,7 @@ PyArray_Round(PyArrayObject *a, int decimals, PyArrayObject *out) | |||
return (PyObject *)out; | |||
} | |||
else { | |||
Py_INCREF(a); | |||
return (PyObject *)a; | |||
return PyArray_Copy(a); |
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.
return PyArray_Copy(a); | |
return PyArray_NewCopy(a, NPY_KEEPORDER); |
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.
Oh... This made the test fail, but I hope that needs a test adjustment only :).
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.
Well, I parroted a PyArray_Copy
from the complex dtype code path of this same function, so if we insist on NPY_KEEPORDER
for real dtypes, we need to follow for complex dtypes, too. Done now.
Otherwise, `round` returns a view for integer arguments and a copy otherwise. All other "rounding" functions (ceil, floor, trunc, rint), always return copies. Thus, make `round` consistent with them.
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.
Thanks a lot @ev-br, sorry for taking so long and thanks for the small follow-ups! Let's give this a shot :).
* BUG: make round consistently return a copy Otherwise, `round` returns a view for integer arguments and a copy otherwise. All other "rounding" functions (ceil, floor, trunc, rint), always return copies. Thus, make `round` consistent with them. * DOC: add a release note snippet for the `round` change * TST: add a test for round preserving the order * MAINT: address review comments * BUG: round: NPY_KEEPORDER for complex arrays, too
Otherwise,
round
returns a view for integer arguments and a copy otherwise. All other "rounding" functions (ceil, floor, trunc, rint), always return copies. Thus, makeround
consistent with the rest of them.fixes #29124