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

fromarray: add type hints #7936

Merged
merged 9 commits into from
Apr 17, 2024
Merged

Conversation

adamjstewart
Copy link
Contributor

Really excited to see type hints in the latest release!

The only feature I'm using that was missing type hints is fromarray, so I added them. Not 100% sure if numpy.typing.ArrayLike is the right way to go, but feel free to suggest alternatives.

@Yay295
Copy link
Contributor

Yay295 commented Apr 2, 2024

I think the actual type would be numpy.ndarray, but that would require importing numpy.

@adamjstewart
Copy link
Contributor Author

It doesn't have to be a numpy.ndarray, it could also be a PIL.Image or any other structure that implements an array interface. There are many ways to avoid importing numpy for typing: https://mypy.readthedocs.io/en/stable/runtime_troubles.html

@Yay295
Copy link
Contributor

Yay295 commented Apr 2, 2024

The other half of the NumPy support, which also isn't typed yet, is here:

Pillow/src/PIL/Image.py

Lines 686 to 709 in aeeb596

@property
def __array_interface__(self):
# numpy array interface support
new = {"version": 3}
try:
if self.mode == "1":
# Binary images need to be extended from bits to bytes
# See: https://github.com/python-pillow/Pillow/issues/350
new["data"] = self.tobytes("raw", "L")
else:
new["data"] = self.tobytes()
except Exception as e:
if not isinstance(e, (MemoryError, RecursionError)):
try:
import numpy
from packaging.version import parse as parse_version
except ImportError:
pass
else:
if parse_version(numpy.__version__) < parse_version("1.23"):
warnings.warn(str(e))
raise
new["shape"], new["typestr"] = _conv_type_shape(self)
return new

@Yay295
Copy link
Contributor

Yay295 commented Apr 2, 2024

I think numpy.typing.ArrayLike is too broad, since it includes scalars and sequences, whereas Image.fromarray() only supports the NumPy array interface.

@nulano
Copy link
Contributor

nulano commented Apr 2, 2024

I think you may need to add numpy to the list of dependencies here

Pillow/pyproject.toml

Lines 69 to 71 in aeeb596

typing = [
'typing-extensions; python_version < "3.10"',
]
or in a new block to fix the mypy error.

@Yay295
Copy link
Contributor

Yay295 commented Apr 2, 2024

Something like

from typing import Protocol, Sequence, TypedDict
class ArrayInterfaceDict(TypedDict):
    shape: Sequence[int]
    typestr: str
class ArrayInterface(Protocol):
    __array_interface__: ArrayInterfaceDict

would be a more exact definition of what Pillow requires.

@nulano
Copy link
Contributor

nulano commented Apr 2, 2024

Something like

from typing import Protocol, Sequence, TypedDict
class ArrayInterfaceDict(TypedDict):
    shape: Sequence[int]
    typestr: str
class ArrayInterface(Protocol):
    __array_interface__: ArrayInterfaceDict

would be a more exact definition of what Pillow requires.

Adding that to PIL._typing would probably be a better option than requiring numpy be installed.

@radarhere
Copy link
Member

__array_interface__ can optionally have "strides", but TypedDict only gained optional items in Python 3.11, so it doesn't look like we can use a TypedDict here.

Otherwise, I've created adamjstewart#1 as a suggestion to add the protocol.

@adamjstewart
Copy link
Contributor Author

Thanks for the help @radarhere!

P.S. Love the profile pic @Yay295, excited for season 3 coming out this weekend!

@@ -3069,7 +3069,15 @@ def frombuffer(mode, size, data, decoder_name="raw", *args):
return frombytes(mode, size, data, decoder_name, args)


def fromarray(obj, mode=None):
class SupportsArrayInterface(Protocol):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since (I assume) this is not intended to be exported, I would expect we either want this to be private, i.e.

Suggested change
class SupportsArrayInterface(Protocol):
class _SupportsArrayInterface(Protocol):

or in a private module, i.e. PIL._typing.SupportsArrayInterface.

@radarhere what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was modelling this after our implementation of SupportsGetMesh.

#7812 (comment)

If we're using _SupportsGetMesh as a type hint, this probably is something users might want to use, so should be accessible as a non-underscore thing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like the difference is that SupportsGetMesh is a Pillow protocol, whereas SupportsArrayInterface seems to be a numpy protocol. Similarly, we have PIL._typing.SupportsRead for a builtin Python protocol. I would therefore expect users to either use the actual numpy type (e.g. numpy.ndarray) or define their own protocol.

@hugovk Do you have any thoughts as the author of the quoted comment?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, let's go for SupportsArrayInterface, it's probably more useful that way, and it's only a small class. We an always deprecate/remove later if needed.

@nulano
Copy link
Contributor

nulano commented Apr 4, 2024

Not 100% sure if numpy.typing.ArrayLike is the right way to go

NumPy seems to overload the np.array function with several types including Any and I can't find a more specific protocol.

src/PIL/Image.py Outdated Show resolved Hide resolved
radarhere and others added 2 commits April 7, 2024 08:32
Co-authored-by: Ondrej Baranovič <ondreko.tiba@gmail.com>
@adamjstewart
Copy link
Contributor Author

Anything else needed for this PR?

@radarhere
Copy link
Member

There's an unresolved debate, but I expect it will be resolved sooner or later before the next release.

@hugovk hugovk merged commit f8160b8 into python-pillow:main Apr 17, 2024
53 of 55 checks passed
@hugovk
Copy link
Member

hugovk commented Apr 17, 2024

Thanks!

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.

5 participants