unsupported indexer reporting in lax_numpy

[curlup]

prints offending indexer value with type and position, instead of printing all of the index

[jakevdp]

Thanks! This looks like it may be useful, but I find that constructing the new error text fails in some cases; for example:

>>> import jax.numpy as jnp
>>> x = jnp.arange(5)
>>> x[{}]
...
AttributeError: 'NoneType' object has no attribute 'dtype'

Can you update to construct the error string more defensively?

[curlup]

should be better now, not sure if you meant it exactly that way though @jakevdp

[jakevdp]

I still think we need to be more defensive. You solved the particular case I mentioned, but I think it's certainly possible that if abstract_i won't have a dtype attribute even if i is None. That check is a bit of an "action at a distance" – it makes assumptions about the relationship between the two variables that is difficult for the reader of the code to ascertain without searching elsewhere in the file. That's kind of an antipattern, because it makes the code brittle and harder to maintain.

What about something like this instead?

       raise IndexError("Indexing mode not yet supported. Got unsupported indexer "
                        f"at position {idx_pos}: {i}")

[curlup]

Sure, totally makes sense, should've thought of that myself (PS I wonder if there could be like a guiding doc for stuff like that. I know it's pretty minimal, but would be nice to have self-service PR self-review enabled by something like that)

[curlup]

Looks good, thanks! The last thing we'll need here is to squash all the changes into a single commit (see https://jax.readthedocs.io/en/latest/contributing.html#single-change-commits-and-pull-requests). Let me know if you need assistance with that – thanks!

I think GH "merge" can do that, doesn't it? Or you want me explicitly push --force the squashed ?

[jakevdp]

I think GH "merge" can do that, doesn't it? Or you want me explicitly push --force the squashed ?

Yes, but we don't use GH merge, and the tool we use doesn't support automatic squashing unfortunately (see #13265 for a discussion). What I'm asking is for you to explicitly force push the squashed commits.

[curlup]

I think GH "merge" can do that, doesn't it? Or you want me explicitly push --force the squashed ?

Yes, but we don't use GH merge, and the tool we use doesn't support automatic squashing unfortunately (see #13265 for a discussion). What I'm asking is for you to explicitly force push the squashed commits.

Thanks for explanation, sorry for so much communication around a tiny improvement 😬 I created new PR with squashed commits and one clean commit message. I hope that works. Closing this in favor of #20666 then? (lol on how the PR numbers look similar)