-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
[Bug]: GridSpec do not type check as Iterable in zip #28151
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
Comments
FWIW, adding the following roll-your-own
|
There are a couple of things that could be done here. I'm not too enthused by adding a new custom object here, as in your example. (It is an additional layer of complexity/public (or at least leaky to public) API. One option would be to lean in to the
Upside is that it is easy to do, and makes it conform to standards. The other option would be to just implement class GridSpec(GridSpecBase):
...
def __iter__(self):
for i in range(self.nrows * self.ncols):
yield self[i] The generator object is effectively the same as the custom class, but returns a standard python type. It would seem to me to be a smaller leap from But it still satisfies type checking, and is slightly (in my opinion) easier to follow what is happening. I think my leaning is to go with adding a |
I guess the typeshed issue you want to link to is python/typeshed#8597. I suspect python/typeshed#7817 may also provide a sufficient workaround. |
An explicit |
Thanks all for sharing, and good to know It doesn't look like the community is going to come to a consensus soon on whether it's a
I'm slightly in favor of the explicit |
Would this be a reasonable workaround? https://github.com/pytorch/pytorch/pull/77116/files It fixes the issue but does not change runtime behavior. So it's only "type"-overhead. AFAICS, the only reason it was not integrated in pytorch is that the respective class is generally mapping-like. While many users use interger indices, which then allow iteration, the behavoir is not guaranteed in general. I think it would always work for GridSpec. |
Since we use It will trigger stubtest to fail since runtime has no This is effectively the same as the pytorch example, just not touching runtime file at all. I will defend the choice made by type checkers here because this style of "iterable" is dependent on a particular meaning applied to |
Bug summary
GridSpec
objects are iterable because they have a__getitem__
method. However, they are not acollections.abc.Iterable
:This means that they do not type check in
zip
, at least according to typeshedCode for reproduction
Actual outcome
Code runs, but type type checking says that GridSpec is not Iterable
Expected outcome
Type checks OK
Additional information
No response
Operating system
Linux
Matplotlib Version
3.8.4
Matplotlib Backend
No response
Python version
3.10.12
Jupyter version
N/A
Installation
pip
The text was updated successfully, but these errors were encountered: