-
Notifications
You must be signed in to change notification settings - Fork 0
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.
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
Added type hints #10
Conversation
| # 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
Manually merged -- without the elt change. |
Suggestions for python-pillow#8908