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

#7805: Ensure all references to mode are Literals, not str. #7807

Merged
merged 2 commits into from
May 8, 2022

Conversation

Julian-O
Copy link
Contributor

@Julian-O Julian-O commented May 8, 2022

Made edits, waiting for automated tests to run.

I am unclear how to actually test the changes - e.g. to make sure the example code in #7805 is now passing without type warnings.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented May 8, 2022

Diff from mypy_primer, showing the effect of this PR on open source code:

vision (https://github.com/pytorch/vision)
+ torchvision/prototype/transforms/functional/_meta.py:156: error: Argument 1 to "convert" of "Image" has incompatible type "str"; expected "Optional[Literal['1', 'CMYK', 'F', 'HSV', 'I', 'L', 'LAB', 'P', 'RGB', 'RGBA', 'RGBX', 'YCbCr']]"  [arg-type]

@srittau srittau linked an issue May 8, 2022 that may be closed by this pull request
@srittau srittau merged commit 032d937 into python:master May 8, 2022
@srittau
Copy link
Collaborator

srittau commented May 8, 2022

Thanks! We have a good set of automated tests in typeshed that already test quite a few things. In fact, mypy primer had a complaint above, but in this case I think pytorch should just adapt its type annotations.

We have a few manual tests in test_cases, but for a fairly simple change as this, we don't need any specific tests.

@Julian-O
Copy link
Contributor Author

Julian-O commented May 8, 2022

Diff from mypy_primer, showing the effect of this PR on open source code:

Hmm... What is going on here? meta_.py makes call to convert (line 156) based on literals listed in line 136-141, one of which is "LA" which is outside the accepted values for _Mode.

Pillow explains that LA is not one of the "standard modes", but that

Pillow also provides limited support for a few additional modes, including:

LA (L with alpha)
[...]

I am not sure what the next step should be,

@hauntsaninja
Copy link
Collaborator

Maybe we should add it to _Mode?

@Julian-O Julian-O deleted the PIllowImageMode branch May 19, 2022 16:19
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.

PIllow: Image.mode should be _Mode, not str
3 participants