-
-
Notifications
You must be signed in to change notification settings - Fork 11.9k
MAINT: Make arrayprint str and repr the ndarray defaults. #17141
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
|
The danger here is that now we can't debug issues with arrays at module load time, as attempting to print them will hit cyclic imports.
|
|
@eric-wieser Why do you think that would be a problem? The original didn't work properly until |
|
If the fallback was really unusable, then I guess it makes no difference. |
numpy/core/src/multiarray/strfuncs.c
Outdated
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 something like this would be better:
NPY_NO_EXPORT PyObject *
array_repr(PyArrayObject *self)
{
if (PyArray_ReprFunction != NULL) {
return PyObject_CallFunctionObjArgs(PyArray_ReprFunction, self, NULL);
}
/*
* We need to do a delayed import here as initialization on module load
* leads to circular import problems.
*/
static PyObject *repr = NULL;
npy_cache_import("numpy.core.arrayprint", "_default_array_repr", &repr);
if (repr != NULL) {
return PyObject_CallFunctionObjArgs(repr, self, NULL);
}
// Chain an explanation onto the `ImportError`, since something has gone very wrong,
// likely due to something already being wrong for someone making changes to numpy.
PyObject *exc, *val, *tb;
PyErr_Fetch(&exc, &val, &tb);
PyErr_Format(PyExc_RuntimeError, "Unable to configure ndarray.__repr__");
npy_PyErr_ChainExceptionsCause(exc, val, tb);
return NULL;
}There's no need to do the import if we've configured a repr function by other means. This also results in a better traceback, without silencing the ImportError.
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.
Note that using master and inserting
array([1,2,3]).__repr__()
in arrayprint before setting the repr function results in a segfault, the current default isn't working.
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, that's good to know. I'm in favor of removing the broken fallback, but I think the above implementation is safer
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 looked at PEP 0490 and implemented a helper function npy_PyExc_SetStringChained. It is local to the strfuncs file but we may want to move it after discussion to npy_3kcompat.
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.
PEP 490 was rejected, PEP 3134 is the one that got accepted. I don't particularly like npy_PyExc_SetStringChained, because:
- It is named after a rejected PEP
- We only use it in two places
- It saves only three lines
- It doesn't generalize well, you end up with a
Chainedvariant for every one of thePyExc_...functions
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.
Umm, AFAICT, PEP 0490 was a followup to PEP 3134 (nee 344) extending it to C code. In any case, there are 30+ uses of npy_cache_import and the thought was that we might want to deal with some of those uses also. If not, we should just follow current practice and return NULL on failure.
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.
Whoops, sorry for accusing you of having the wrong PEP.
In any case, there are 30+ uses of
npy_cache_importand the thought was that we might want to deal with some of those uses also.
That's a fair argument. I'd maybe consider this failing import special, as someone who hits it is likely already trying to debug a bigger issue due to a change they made. Either is fine by me though - both are better than your original patch which discarded the ImportError.
This removes the old default routines in 'strfunc.c' that were never used and looked to have unicode/byte problems. The functions now self initialize when called, so the explicit initialization when the arrayprint module is loaded is no longer needed.
6fa53ff to
4da857a
Compare
|
Thanks @charris |
|
|
||
|
|
||
| static void | ||
| npy_PyErr_SetStringChained(PyObject *type, const char *message) |
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.
_PyErr_FormatFromCause seems to be the name used internally by python, added in python/cpython@467ab19
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 that's a better name.
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.
Since we use the limited API, I think we can just use _PyErr_FormatFromCause directly here and elsewhere.
This removes the old default routines in 'strfunc.c' that were never
used and looked to have unicode/byte problems. The functions now
self initialize from arrayprint when called, so the explicit initialization
previously done when arrayprint module was loaded is no longer needed.
Closes #17138.