-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Allow getpixel() to accept a list #7355
Conversation
src/_imaging.c
Outdated
@@ -1146,11 +1146,15 @@ static inline int | |||
_getxy(PyObject *xy, int *x, int *y) { | |||
PyObject *value; | |||
|
|||
if (!PyTuple_Check(xy) || PyTuple_GET_SIZE(xy) != 2) { | |||
int tuple = PyTuple_Check(xy); |
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.
So many checks here. Isn't a way do something like PyArg_ParseTuple(xy, "ii", x, y)
?
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.
Possibly - but if your goal is simplicity, then I should mention that the simplest solution is to implement this change in Python instead of C.
diff --git a/src/PIL/Image.py b/src/PIL/Image.py
index 476ed0122..6ea711b56 100644
--- a/src/PIL/Image.py
+++ b/src/PIL/Image.py
@@ -1565,7 +1565,7 @@ class Image:
self.load()
if self.pyaccess:
return self.pyaccess.getpixel(xy)
- return self.im.getpixel(xy)
+ return self.im.getpixel(tuple(xy))
def getprojection(self):
"""
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.
My original intention here was to make this change close to the source of the error though. If the change is being made higher up, then my argument that it would be awkward to update the error message for just this scenario falls apart, since you could just add a new error message.
Then I feel less sure about this change, as the discourse in this issue doesn't have a clear conclusion, and I'm reminded of #3738, in particular #3738 (comment) and #3738 (comment).
Also, tuples and lists are for slightly different things (immutable/mutable, structure/order). Does that really matter? I think maybe it does: semantically you know a coordinate like (100, 200) can't have a third value; but [100, 200] looks like it could. So I'm tending towards keeping it as is.
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 didn't have strong feelings in #3738 (comment), but in the years since then am now tending the other way, maybe we could accept more iterables for these kinds of inputs?
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.
Ok, I've pushed a commit with a simpler change in Python.
src/PIL/Image.py
Outdated
if isinstance(xy, list): | ||
xy = tuple(xy) | ||
return self.im.getpixel(xy) |
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.
Unconditional casting is faster and makes possible other types which was asked.
In [1]: a = (1, 2)
In [3]: %timeit _ = tuple(a)
29.4 ns ± 0.059 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)
In [4]: %%timeit
...: if isinstance(a, list):
...: _ = tuple(a)
51.9 ns ± 0.0765 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)
In [2]: b = [1, 2]
In [5]: %timeit _ = tuple(b)
42.8 ns ± 0.0378 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)
In [6]: %%timeit
...: if isinstance(b, list):
...: _ = tuple(b)
83.4 ns ± 0.235 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)
if isinstance(xy, list): | |
xy = tuple(xy) | |
return self.im.getpixel(xy) | |
return self.im.getpixel(tuple(xy)) |
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.
In main at the moment, im.getpixel(0)
raises TypeError: argument must be sequence of length 2
Your suggestion would raise instead TypeError: 'int' object is not iterable
I was trying to avoid that change. Perhaps it is not a significant concern however.
Co-authored-by: Alexander Karpinsky <homm86@gmail.com>
Thanks! |
Resolves #7352
The issue points out that while
im.getpixel()
accepts a tuple,im.getpixel((0, 0))
, it does not accept a list.Even though the list supplied is still a "sequence of length 2".
It is worth noting that this error string is shared with another situation.
So the error message cannot be simply reworded to say "tuple", as I don't think that clearly explains the problem with
im.load()[0, 0, 0]
.Instead, this PR allows
im.getpixel()
to accept a list.