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

Behaviour of E721 with numpy.dtype #9570

Closed
nstarman opened this issue Jan 17, 2024 · 8 comments · Fixed by #9676
Closed

Behaviour of E721 with numpy.dtype #9570

nstarman opened this issue Jan 17, 2024 · 8 comments · Fixed by #9676
Assignees
Labels
accepted Ready for implementation rule Implementing or modifying a lint rule

Comments

@nstarman
Copy link

nstarman commented Jan 17, 2024

numpy.dtype is tripping rule E721. I'm not sure it should be.

x = np.array([1,2,3], dtype=float)
x.dtype == float  # <- trips E721

.dtype is an object, e.g. a dtype('float64').
While x.dtype == float may not be the best means of testing a dtype, I don't think this is E721.

@charliermarsh
Copy link
Member

What about x.dtype is np.dtype(float)?

@charliermarsh charliermarsh added the question Asking for support or clarification label Jan 18, 2024
@nstarman
Copy link
Author

nstarman commented Jan 18, 2024

Looking at https://github.com/numpy/numpy/blob/5feea41396910a3065c8e79886127fad4cbdebf3/numpy/__init__.pyi#L844, it appears that == is fine for dtypes.

I suppose we should ask a numpy dev — ping @mhvk — about preferred syntax.

@mhvk
Copy link

mhvk commented Jan 18, 2024

dtype are a bit of a strange beast, but definitely best thought of as instances, not classes, and they are meant to be comparable not just to their own class, but also to the corresponding scalar types (e.g., x.dtype == np.float32) and strings (e.g., x.dtype == ['i1,i4']; basically, __eq__ always tries to do dtype(other).

@charliermarsh
Copy link
Member

Okay, we can try to special-case it when comparing a .dtype attribute directly, though we'll still get it wrong in the general case (since given x == y, we can't know definitively whether x is a dtype).

@charliermarsh charliermarsh added rule Implementing or modifying a lint rule and removed question Asking for support or clarification labels Jan 19, 2024
@charliermarsh
Copy link
Member

@mhvk -- Just to clarify, is x.dtype == float is preferable over (e.g.) x.dtype is np.dtype(float) or looking at the dtype kind or char?

@zanieb
Copy link
Member

zanieb commented Jan 19, 2024

I feel like x.dtype is np.dtype(float) is more correct, but this might make sense to exclude from E721 as it doesn't seem worse to use == float.

@mhvk
Copy link

mhvk commented Jan 19, 2024

I think it is much safer in general usage to use ==, since is really only works for the basic numpy built-in dtype. np.dtype("i,i") is np.dtype("i,i") gives False, because a new structured dtype is created for every call. So, to the point of this PR, dtype really should not generally be considered classes and thus no preference for is over isinstance or ==.

@charliermarsh
Copy link
Member

Okay, sounds good. Thanks for chiming in.

@charliermarsh charliermarsh added the accepted Ready for implementation label Jan 29, 2024
@charliermarsh charliermarsh self-assigned this Jan 29, 2024
charliermarsh added a commit that referenced this issue Jan 29, 2024
## Summary

Per #9570:

> `dtype` are a bit of a strange beast, but definitely best thought of
as instances, not classes, and they are meant to be comparable not just
to their own class, but also to the corresponding scalar types (e.g.,
`x.dtype == np.float32`) and strings (e.g., `x.dtype == ['i1,i4']`;
basically, `__eq__` always tries to do `dtype(other)`.

This PR thus allows comparisons to `dtype` in preview.

Closes #9570.

## Test Plan

`cargo test`
zanieb pushed a commit that referenced this issue Jan 29, 2024
## Summary

Per #9570:

> `dtype` are a bit of a strange beast, but definitely best thought of
as instances, not classes, and they are meant to be comparable not just
to their own class, but also to the corresponding scalar types (e.g.,
`x.dtype == np.float32`) and strings (e.g., `x.dtype == ['i1,i4']`;
basically, `__eq__` always tries to do `dtype(other)`.

This PR thus allows comparisons to `dtype` in preview.

Closes #9570.

## Test Plan

`cargo test`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Ready for implementation rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants