BUG: ensure _UFuncNoLoopError can be pickled#17377
BUG: ensure _UFuncNoLoopError can be pickled#17377seberg merged 4 commits intonumpy:masterfrom bfgray3:ufuncnolooperror-pickling
Conversation
bfgray3
changed the title
|
Still need to adjust the doctest that is failing |
| class TestUFuncNoLoopError: | ||
| def test_pickling(self): | ||
| """ Test that _UFuncNoLoopError can be pickled """ | ||
| assert isinstance(pickle.dumps(_UFuncNoLoopError), bytes) |
There was a problem hiding this comment.
Thanks for looking into this, looks good to me. If you have some time, maybe you can extend the test to also loading and the error instance and not just the error class?
It may be easier for memory error (either seems enough to me for now). Could you add a test that round-tripping works, maybe something like:
error = _ArrayMemoryError((1023,), np.dtype(np.uint8))
res = pickle.loads(pickle.dumps(error))
assert res._total_size == error._total_size
I would mostly like a test that the error (instances) can also be unpickled again. I admit, they clearly can here.
There was a problem hiding this comment.
Thanks. One more nitpick: test would make more sense as a test_pickling in the TestArrayMemoryError class.
| Traceback (most recent call last): | ||
| ... | ||
| numpy.core._exceptions.UFuncTypeError: Cannot cast ufunc 'add' output from dtype('float64') to dtype('int64') with casting rule 'same_kind' | ||
| numpy.core._exceptions._UFuncOutputCastingError: Cannot cast ufunc 'add' output from dtype('float64') to dtype('int64') with casting rule 'same_kind' |
There was a problem hiding this comment.
Hmm, @eric-wieser I think you had an opinion on the original issue? I had tried in ipython, and I thought it just printed the __name__, but maybe not.
I think I will just open an issue about it after this is done, but it would seem nice if we can find a solution and also print out a name that we want users to use in try: ... except: Error. UFuncTypeError seems good, but it should not be in a private module, and maybe we should just use a single class for all the subclasses (at least until we make them public), to avoid printing a private name?
There was a problem hiding this comment.
maybe we should just use a single class for all the subclasses (at least until we make them public), to avoid printing a private name?
One of the ideas behind the subclasses was to avoid actually paying any string formatting costs until the traceback is printed. Merging them into one class would make that more difficult.
There was a problem hiding this comment.
OK, maybe the real question is different: If UFuncTypeError is private (due to being defined in _exceptions), should this set it to TypeError.__qualname__ and TypeError.__name__, etc. instead?
It might not be hard to fix this differently by using __reduce__(self): return f"{original_module}.{original_qualname}".
There was a problem hiding this comment.
Overloading __reduce__ sounds promising to me
There was a problem hiding this comment.
I guess the question is whether right now we are OK with messing up the print slightly to fix the pickling. Sorry, I thought this was much clearer :(.
I am not sure __reduce__ will work easily. It will be a solution for the error instance, but I doubt it will work for the error class. Now the original bug report did not have an issue with that, so maybe we can get away with a class that cannot be pickled (but its instances can).
|
@seberg, @eric-wieser is this good to go? |
|
Arrg, sorry. I am +0.5 for putting it in. The printing gets slightly worse, but it is already bad with There is some "proper" fix missing here, but right now I am not sure what it is. Maybe exposing a |
seberg
added
the
triage review
|
OK, this is not ideal with printing. But it is a possible regression in some use cases, so putting it in (we discussed it briefly on the call). Thanks! |
* BUG: ensure _UFuncNoLoopError can be pickled; closes numpy#16490 * update quickstart.rst * add round trip pickle test * move _ArrayMemoryError picking test
5 participants
Applies the suggested solution in #16490 and adds a simple test.