Skip to content

ENH: Slightly improve error when gufunc axes has wrong size#22675

Merged
seberg merged 4 commits intonumpy:mainfrom
seberg:slight-error-improvement-axes
Nov 28, 2022
Merged

ENH: Slightly improve error when gufunc axes has wrong size#22675
seberg merged 4 commits intonumpy:mainfrom
seberg:slight-error-improvement-axes

Conversation

@seberg
Copy link
Copy Markdown
Member

@seberg seberg commented Nov 25, 2022

My hope was to tweak it into something useful that:

a @= b

can raise when b should have two dimensions and has two axes specified but actually only has one.
I didn't succeed, but I still think it a slight improvement to give the ufunc name and the actual core dimensions.

Copy link
Copy Markdown
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good to be more explicit! One super nitpicky comment; can ignore if you wish.

My hope was to tweak it into something useful that:

    a @= b

can raise when `b` should have two dimensions and has two axes specified
but actually only has one.
I didn't succeed, but I still think it a slight improvement to give the
ufunc name and the actual core dimensions.
@seberg seberg force-pushed the slight-error-improvement-axes branch from 04d7e37 to 5e15682 Compare November 25, 2022 19:16
@seberg
Copy link
Copy Markdown
Member Author

seberg commented Nov 25, 2022

@mhvk one thing I noticed looking here, is that if core axes are optional, you cannot omit them by passing a shorter axes tuple. I.e. matul(2D, 2D, axes=[(-2, -1)], (-1,), (-1,)]) doesn't work. Just curious if it is intentional, or because it would be complicated to allow?

@mhvk
Copy link
Copy Markdown
Contributor

mhvk commented Nov 25, 2022

@seberg - I think I introduced the ability to give axes just before we allowed for optional axes, so I think it is more a question of oversight...

It might actually be quite neat if one could indeed use axes to signal one really wanted matrix-stack @ vector-stack rather than a stack of matrices. Though perhaps (mat @ vec[..., np.newaxis]).squeeze(-1) or just np.einsum(...) ends up being clearer. And the alternative of just implementing matvec and vecmat is perhaps cleanest of all.

@mhvk
Copy link
Copy Markdown
Contributor

mhvk commented Nov 25, 2022

p.s. This seems all OK, so fine to merge.

@mattip
Copy link
Copy Markdown
Member

mattip commented Nov 26, 2022

Could these be AxisError instead of ValueError ?

@seberg
Copy link
Copy Markdown
Member Author

seberg commented Nov 28, 2022

Hmmm, right now we only use AxisError for an out-of-bounds axis I think, not for a different way axis= can be invalid.

So yeah, makes sense, although for myself I am happy to not broaden up AxisError for this (happy to do so I am just ±0 on it).

@mattip
Copy link
Copy Markdown
Member

mattip commented Nov 28, 2022

Something other than ValueError would be helpful in order to properly catch errors in a @= b

… entries

This allows catching the error relatively targeted for in-place matmul `a @= b`
which may use this path.
@seberg
Copy link
Copy Markdown
Member Author

seberg commented Nov 28, 2022

Heh, I forgot about that idea... I guess that is a good enough reason here, we might want to do it for more axis/axes related ValueError's I guess.

@mattip
Copy link
Copy Markdown
Member

mattip commented Nov 28, 2022

This is failing. It used to raise TypeError("axes item 2 should be a tuple").

a = np.arange(27.).reshape((3, 3, 3))
b = np.arange(10., 19.).reshape((3, 1, 3))
assert_raises(TypeError, inner1d, a, b, axes=[-1, -1, -1])

@seberg
Copy link
Copy Markdown
Member Author

seberg commented Nov 28, 2022

Grrr, restored the TypeError where it clearly is correct the long way around... Not pretty, but maybe the practical path?

Copy link
Copy Markdown
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better logic indeed, even if a bit more tortuous.

Copy link
Copy Markdown
Member

@mattip mattip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I added a release note, please take a look before merging.

@seberg
Copy link
Copy Markdown
Member Author

seberg commented Nov 28, 2022

Thanks Matti, I am not sure we need the note for such a niche change, but it reads well and can't hurt.

@seberg seberg merged commit 4dc5321 into numpy:main Nov 28, 2022
@seberg seberg changed the title DOC: Slightly improve error when gufunc axes has wrong size ENH: Slightly improve error when gufunc axes has wrong size Nov 28, 2022
@seberg seberg deleted the slight-error-improvement-axes branch November 28, 2022 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants