Skip to content

Conversation

jbrockmendel
Copy link
Member

xref #62423, does not close.

This does not take a stand on whether to support list/tuple in the EA methods. An EA that currently supports it will continue to, likewise for arrays that don't support it.

Not taking that stand limits the amount of de-duplication/centralization we can do. Instead of all the exceptions being raised from unpack_zerodim_and_defer, helpers ops.get_op_exception_message and ops.get_shape_exception_message should at least help us get to consistent messages.

I did a pass to try to use these new helpers where relevant, but definitely didn't get them all. I also didn't implement _supports_(scalar|array)_op yet.

Before I go through the tests to update the exception messages, I'd like to get any bikeshedding out of the way about exactly what those messages should say.

@jbrockmendel
Copy link
Member Author

Brainstorming potential improvements to the messages:

  1. for reversed ops e.g __rsub__(x, y), we can make the message about sub(y, x) instead?
  2. In some places we have "do x instead" in the exception message. We could let the pattern accomodate that by having _supports_foo_op return tuple[bool, str | None] with the latter being an optional bit to add to the standard exception message.

Copy link
Member

@rhshadrach rhshadrach left a comment

Choose a reason for hiding this comment

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

Really like the consolidation here.

for reversed ops e.g rsub(x, y), we can make the message about sub(y, x) instead?

I think this would be good, but not a necessity; I believe the error message will be understandable even if the order doesn't line up.

We could let the pattern accomodate that by having _supports_foo_op return tuple[bool, str | None]

Having some trouble seeing many cases today where it would be used, but having the flexibility to do so seems good.

Comment on lines +38 to +41
if left.ndim == right.ndim == 1:
return "Lengths must match"
else:
return "Shapes must match"
Copy link
Member

Choose a reason for hiding this comment

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

Can we report on the lengths / shapes here?

@jbrockmendel
Copy link
Member Author

@jorisvandenbossche thoughts on what the One True Exception Message should be?

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.

2 participants