-
-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
DEP: Complete deprecation of invalid array/memory order #14596
Conversation
return NPY_SUCCEED; | ||
/* 2015-12-14, 1.11 DEPRECATED */ | ||
/* 2019-09-25, 1.18 ERROR */ | ||
PyErr_SetString(PyExc_ValueError, "Non-string object detected for " |
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.
I think for errors I prefer the formatting:
PyErr_SetString(PyExc_ValueError,
"this is a very long error message .... ... ... .. "
"...");
As for the above comment, I do not think we typically put that in, not that it hurts, though, hmmm. The idea of the DEPRECATED comment is to find these places that need updating easier. What is important here is only the release note.
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.
I updated the PyErr_SetString, I'm happy to
- remove the both dates
- consolidate them into a single line
- /* more order formats were handled before DEP: Complete deprecation of invalid array/memory order #14596 */
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.
Others may disagree, but if you do not mind, lets just remove the comments, they are not interesting internally for numpy, only for downstream. And downstream would read the release notes.
Can you reformat the other error setting as well?
The main thing to do is add the release note snippet, thanks!
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.
Removed comments and added a release note snippet.
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. LGTM. Do you want to squash it into a single commit with DEP: ...
? I can also squash merge, but thought I would mention just in case, since we slightly prefer merges.
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.
I'm happy to do what you prefer.
I'm squashing and force committing a single commit with the correctish title
Interesting. The test failures seem real. There must be some documentation around which actually uses the deprecated version, hah... |
f1c9fc3
to
093bc62
Compare
Fixed order=FORTRAN in docs Added release snippet
093bc62
to
6177445
Compare
There's one test failure in windows for |
It's a Heisenbug that shows up randomly, usually on windows 32 bits. |
Hmmm, there once was a bug with EDIT: Anyway, made a PR for OpenBLAS, if we are lucky, the next update fixes it, I guess... |
Thanks @sethtroisi |
# invalid after gh-14596 | ||
for order in ['Z', 'c', False, True, 0, 8]: | ||
x = np.array([[1, 2, 3], [4, 5, 6]], np.int32) | ||
assert_raises(ValueError, x.flatten, {"order": order}) |
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.
This test is unfortunately nonsense, this passes a dict as the order argument.
Original deprecation was submitted in #6823 in 2015.