Skip to content

Added type hints #10

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.

Sign up for GitHub

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

Closed
radarhere wants to merge 2 commits into from
Closed

Added type hints #10

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
78887f6
Corrected comment
radarhere May 9, 2025
fd093e3
Added type hints
radarhere May 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .ci/requirements-mypy.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ IceSpringPySideStubs-PySide6
ipython
numpy
packaging
pyarrow-stubs
pytest
sphinx
types-atheris
Expand Down
24 changes: 14 additions & 10 deletions Tests/test_pyarrow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,11 @@
is_big_endian,
)

pyarrow = pytest.importorskip("pyarrow", reason="PyArrow not installed")
TYPE_CHECKING = False
if TYPE_CHECKING:
import pyarrow
else:
pyarrow = pytest.importorskip("pyarrow", reason="PyArrow not installed")

TEST_IMAGE_SIZE = (10, 10)

Expand Down Expand Up @@ -94,14 +98,14 @@ def _test_img_equals_int32_pyarray(
("HSV", fl_uint8_4_type, [0, 1, 2]),
),
)
def test_to_array(mode: str, dtype: Any, mask: list[int] | None) -> None:
def test_to_array(mode: str, dtype: pyarrow.DataType, mask: list[int] | None) -> None:
img = hopper(mode)

# Resize to non-square
img = img.crop((3, 0, 124, 127))
assert img.size == (121, 127)

arr = pyarrow.array(img)
arr = pyarrow.array(img) # type: ignore[call-overload]
_test_img_equals_pyarray(img, arr, mask)
assert arr.type == dtype

Expand All @@ -118,8 +122,8 @@ def test_lifetime() -> None:

img = hopper("L")

arr_1 = pyarrow.array(img)
arr_2 = pyarrow.array(img)
arr_1 = pyarrow.array(img) # type: ignore[call-overload]
arr_2 = pyarrow.array(img) # type: ignore[call-overload]

del img

Expand All @@ -136,8 +140,8 @@ def test_lifetime2() -> None:

img = hopper("L")

arr_1 = pyarrow.array(img)
arr_2 = pyarrow.array(img)
arr_1 = pyarrow.array(img) # type: ignore[call-overload]
arr_2 = pyarrow.array(img) # type: ignore[call-overload]

assert arr_1.sum().as_py() > 0
del arr_1
Expand All @@ -152,17 +156,17 @@ def test_lifetime2() -> None:


class DataShape(NamedTuple):
dtype: Any
dtype: pyarrow.DataType
# Strictly speaking, elt should be a pixel or pixel component, so
# list[uint8][4], float, int, uint32, uint8, etc. But more
# correctly, it should be exactly the dtype from the line above.
elt: Any
elt: list[int] | float
Copy link
Owner

@wiredfool wiredfool May 9, 2025

Choose a reason for hiding this comment

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

I don't think this accurately reflects the type intention of elt.

Copy link
Author

@radarhere radarhere May 10, 2025

Choose a reason for hiding this comment

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

elt is used in the file as

assert pixel[ix] == arr[y * img.width + x].as_py()[elt]
arr[(y * img.width + x) * elts_per_pixel + elt]
arr_pixel_tuple[elt]
pyarrow.array([elt] * (ct_pixels * elts_per_pixel), type=dtype)

I acknowledge my understanding of this is not high, but I'm not seeing evidence that it's being used as anything more complex than initial data for a pyarrow array or for index access.

Copy link
Owner

@wiredfool wiredfool May 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

As the comment says, it's a pixel or pixel component -- used for initialization of the data from the datashape tuple.

In the file from all of the cases of DataShape initialization, it's used as:

    elt=[1, 2, 3, 4],  # array of 4 uint8 per pixel
    elt=3,  # one uint8,
    elt=0xABCDEF45,  # one packed int, doesn't fit in a int32 > 0x80000000
    elt=0x12CDEF45,  # one packed int
    3
    1 << 24
    3.14159

The comment listed is somewhat incorrect, as it's the python representation of whatever's in the dtype element. How to express that in a type system, I'm not sure.

I suspect that the types pass the checker because an int is a specific case of a float, but it's misleading.

Copy link
Author

@radarhere radarhere May 10, 2025

Choose a reason for hiding this comment

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

https://peps.python.org/pep-0484/

when an argument is annotated as having type float, an argument of type int is acceptable

Copy link
Owner

@wiredfool wiredfool May 12, 2025

Choose a reason for hiding this comment

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

While it may be permissible, it doesn't clarify the intent of the code. The only point of a type annotation here is to help the next person to edit this, and if the type apparently conflicts with the usage of it it's going to raise more questions than it's going to answer.

elts_per_pixel: int


UINT_ARR = DataShape(
dtype=fl_uint8_4_type,
elt=[1, 2, 3, 4], # array of 4 uint 8 per pixel
elt=[1, 2, 3, 4], # array of 4 uint8 per pixel
elts_per_pixel=1, # only one array per pixel
)

Expand Down
Loading