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

MAINT: Changed the NBitBase variancy in number from co- to invariant #18174

Merged
merged 5 commits into from
Jan 19, 2021

Conversation

BvB93
Copy link
Member

@BvB93 BvB93 commented Jan 15, 2021

As was noted in #18128 (comment), the currently covariant nature of np.number[~NBit_co] caused the likes of np.float32 to be a sub-type of np.float64, behavior that is very much undesirable.

This PR attempts to fix aforementioned issue by making np.number invariant w.r.t. its numerical precision.
Fortunately, this variancy change could be made quite safely and only a single test needed actual updating (08b32c3).

Examples

The behavior prior to this PR.

import numpy as np

def func(a: float64) -> None: ...

f2 = np.float16()
f4 = np.float32()
f8 = np.float64()

# These all pass without error as they are sub-types of `float64` (a.k.a. `floating[_64Bit]`)
func(f2)
func(f4)
func(f8)

@BvB93
Copy link
Member Author

BvB93 commented Jan 15, 2021

Pinging @seberg and @a-reich.
Continuing from #18128 (comment), it turns out that the np.number variancy could be changed without issue.

@a-reich
Copy link

a-reich commented Jan 17, 2021

@BvB93 Nice! Looks like having number invariant is not too bad after all, if that was the only substantial change. I thought e.g. arithmetic operators might need to get messier, but this version is just as clean (mixed-precision ops returned Unions before anyway so it's just a different Union EDIT: whoops I forgot that with covariance mypy simplifies the Union to just one dtype - sorry).
If this gets merged, then having ndarray covariant in dtype makes more sense. (I guess users could still do incorrect assignments on arrays of abstract dtype, but that's fine and will probably be rare).

@charris
Copy link
Member

charris commented Jan 19, 2021

Needs rebase.

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