Skip to content
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

ENH: Clarify error message for invalid array indices #26120

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

YinonHorev
Copy link

This enhancement updates the error message thrown during array indexing
with invalid objects. Previously, the message did not explicitly mention
that tuples of valid indexing objects are also acceptable. This could lead
to confusion, especially when debugging legacy code or for users less
familiar with NumPy's indexing rules.

The new error message now includes "tuples of these objects" to clearly
indicate that tuples containing any combination of valid indices (integers,
slices, ellipsis, numpy.newaxis, and integer or boolean arrays) are valid
indexing methods. This change aims to reduce misunderstanding and improve
the developer experience by providing more direct guidance on valid
indexing types.

See ticket #26115

enhance indexing error message
@mattip
Copy link
Member

mattip commented Mar 24, 2024

I am not sure this is an improvement, the message is getting too long. Maybe instead of listing everything that is accepted, reflect that this particular construct is not accepted, and do the C-equivalent of

f"Index error: cannot index with {type(arr)}"

@seberg
Copy link
Member

seberg commented Mar 25, 2024

I agree with Matti that it would be cool to print out the type of the object the user was using in the last branch (and possibly such a new branch)!

We could maybe be a bit more specific even? If PyArray_NDIM(arr) != 0 then we assume that something (e.g. a list) got converted to an advanced indexing array. I.e. we can add a second branch here to give an entirely more specific error message, where we know the note added here is more likely to apply.
(It is more like the first branch here that warns about only integer and boolean arrays are supported.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants