Skip to content

API: Do not consider subclasses for NEP 50 weak promotion #26905

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

Merged
merged 2 commits into from
Jul 25, 2024

Conversation

seberg
Copy link
Member

@seberg seberg commented Jul 10, 2024

This disables remaining checks for subclasses of floats. We only
apply the weak rules to literals, and thus just ignore subclasses.

Closes gh-26852

Draft because it is based on gh-26904

@seberg seberg marked this pull request as draft July 10, 2024 15:14
@seberg seberg added this to the 2.1.0 release milestone Jul 12, 2024
@seberg seberg marked this pull request as ready for review July 24, 2024 18:34
This disables remaining checks for subclasses of floats.  We only
apply the weak rules to literals, and thus just ignore subclasses.
@seberg seberg force-pushed the strict-typing-nep50 branch from 1d9b38c to f3b669d Compare July 24, 2024 18:35
Copy link
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.

I guess this is because isinstance(np.float64(1.), float) is True, right? I guess in principle another float subclass might exist for which weak promotion might make sense, and an alternative might be to explicitly exclude np.generic subclasses. But since that only makes things slower, probably best to do what you have here! (We can always revisit if the need arises.)

@ngoldbaum
Copy link
Member

I think the build failures on the OpenBLAS CI are real. Might just be a matter of giving result in the convert_to_@name@ template a default value?

@seberg
Copy link
Member Author

seberg commented Jul 25, 2024

Fun that it suddenly shows up, it isn't new. Anyway, harmless but not wrong, easy enough to avoid.

@seberg
Copy link
Member Author

seberg commented Jul 25, 2024

I guess this is because isinstance(np.float64(1.), float) is True, right?

This is a subtlety that we might otherwise run into in Python, I guess. So a bit :).
I guess I explained my reasoning in the issue (and meetings more) only:

  • I think NEP 50 is really about convenience of using Python literals in code. Subclasses are too niche to be very relevant there, IMO.
  • Subclasses can have any odd meaning (including something like float64 that we don't even know about). Treating them like Python float is honestly probably wrong at least half the time. (Of course we do end up treating them like float64, which isn't really better).

Either way, in some future a custom DType could probably create custom weak scalars if that comes up. But that would have complicated things (and this was difficult enough). Once legacy promotion paths are gone, I suspect one could attack it. Although, I honestly doubt it is worthwhile to do it even if we can.

@mhvk
Copy link
Contributor

mhvk commented Jul 25, 2024

Thanks, that all makes sense!

@mattip mattip merged commit 0819378 into numpy:main Jul 25, 2024
68 checks passed
@mattip
Copy link
Member

mattip commented Jul 25, 2024

Thanks @seberg

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

Successfully merging this pull request may close these issues.

API: Make weak promotion only trigger for exact types
4 participants