MAINT: random: Use 'from exc' when raising a ValueError in choice. #18439
Conversation
numpy/random/_generator.pyx
Outdated
| @@ -700,7 +700,7 @@ cdef class Generator: | |||
| # __index__ must return an integer by python rules. | |||
| pop_size = operator.index(a.item()) | |||
| except TypeError: | |||
| raise ValueError("a must an array or an integer") | |||
| raise ValueError("a must be an array or an integer") from None | |||
There was a problem hiding this comment.
Would it make sense to add the type?
| raise ValueError("a must be an array or an integer") from None | |
| raise ValueError(f"a must be an array or an integer not '{type(a)}'") from None |
There was a problem hiding this comment.
Good idea, but at this point, a has already been converted to a numpy array, so type(a) will always be <class 'numpy.ndarray'>. I pushed a change to avoid that problem. (I also fixed a couple more error messages that were missing a space.)
There was a problem hiding this comment.
This is still confusing though, because choice(np.array(1)) will give a message about ndarray not being an array
There was a problem hiding this comment.
I changed the message to say that a must be a sequence or an integer (i.e. replace "array" with "sequence"). Suggestions for better message are welcome!
Use 'from None' when the ValueError is raised when the first argument to `choice` is not an array and not an integer. Also fix the error message for the ValueError, and add a missing space to two other error messages.
WarrenWeckesser
force-pushed
the
random-choice-exc
branch
from
46c1a14
to
4de1259
Compare
numpy/random/_generator.pyx
Outdated
| raise ValueError("a must be a positive integer unless no" | ||
| "samples are taken") | ||
| " samples are taken") |
There was a problem hiding this comment.
I think we normally format this with the space at the end of the prevous line
There was a problem hiding this comment.
Done. If this is important enough to comment on in a PR, perhaps it is important enough to have in a style guide somewhere?
numpy/random/_generator.pyx
Outdated
| a = np.array(a, copy=False) | ||
| if a.ndim == 0: | ||
| try: | ||
| # __index__ must return an integer by python rules. | ||
| pop_size = operator.index(a.item()) | ||
| except TypeError: | ||
| raise ValueError("a must an array or an integer") | ||
| raise ValueError("a must be an array or an integer, " | ||
| f"not {type(a_original)}") from None |
There was a problem hiding this comment.
I'm not convinced the from None is a good idea. Consider:
class MyObject:
def __index__(self):
if some_complex_condition:
raise TypeError("with useful information")
return 42or
class MyObject:
def __index__(self):
return 1 + "2" # oops, TypeErrorWith this PR, the traceback of choice(MyObject()) no longer shows me error information. I think from None should be used very sparingly (eg, for KeyError where you really know exactly what the cause was).
There was a problem hiding this comment.
Do you recommend using except TypeError as exc and then raise ValueError(...) from exc, or just leave it as it is in master?
There was a problem hiding this comment.
from exc sounds good to me.
WarrenWeckesser
changed the title
|
I'm not sure why the pypy test for the exception raised by Wild speculation: Is that a pypy bug, where it loses the format spec when concatenating a regular string and an f-string? |
It seems PyPy does not like cythonized f-strings where the format is Edit: the issue was fixed and it will be part of the next release. |
|
Thanks Warren. |
|
@mattip Any idea how close the next PyPy release is? If it is a ways off I will probably fix the failure. |
|
It might take till April. In the mean time I submitted a PR to use the nightly builds. |
Use 'from exc' when the ValueError is raised when the first argument to
choiceis not an array and not an integer.Also fix the error message for the ValueError.