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

Support list of list in draw.line #3738

Closed
gaoxinge opened this issue Mar 22, 2019 · 11 comments
Closed

Support list of list in draw.line #3738

gaoxinge opened this issue Mar 22, 2019 · 11 comments

Comments

@gaoxinge
Copy link

Code below is running normally:

from PIL import Image, ImageDraw

WHITE = (255, 255, 255)
BLUE = "#0000ff"
MyImage = Image.new('RGB', (600, 400), WHITE)
MyDraw = ImageDraw.Draw(MyImage)
line_points = [(100, 100), (150, 200), (300, 100), (500, 300)]  # list of tuple
MyDraw.line(line_points, width=40, fill=BLUE, joint='curve')
MyImage.show()

But if I change

line_points = [(100, 100), (150, 200), (300, 100), (500, 300)]  # list of tuple

into

line_points = [[100, 100], [150, 200], [300, 100], [500, 300]]  # list of list

the code will raise exception because the points are represented by list.

Is it appropriate that draw.line support the list of list as arguments?

My pillow version is 5.4.1.

@radarhere radarhere changed the title support list of list in draw.line Support list of list in draw.line Mar 22, 2019
@radarhere
Copy link
Member

I can see that it could be useful. I've created PR #3739 to resolve this.

@gaoxinge
Copy link
Author

@radarhere thanks!

@hugovk
Copy link
Member

hugovk commented Mar 26, 2019

My initial thought is how about all the other places we have parameters that take a tuple (eg. Image.new("RGB", size=(20, 20), color=(255, 255, 255)))? Should they also take lists?


A quick search on when to use a tuple or a list gives https://stackoverflow.com/a/1708538/724176:

Tuples are fixed size in nature whereas lists are dynamic.
In other words, a tuple is immutable whereas a list is mutable.

  1. You can't add elements to a tuple. Tuples have no append or extend method.
  2. You can't remove elements from a tuple. Tuples have no remove or pop method.
  3. You can find elements in a tuple, since this doesn’t change the tuple.
  4. You can also use the in operator to check if an element exists in the tuple.

  • Tuples are faster than lists. If you're defining a constant set of values and all you're ever going to do with it is iterate through it, use a tuple instead of a list.

  • It makes your code safer if you “write-protect” data that does not need to be changed. Using a tuple instead of a list is like having an implied assert statement that this data is constant, and that special thought (and a specific function) is required to override that.

  • Some tuples can be used as dictionary keys (specifically, tuples that contain immutable values like strings, numbers, and other tuples). Lists can never be used as dictionary keys, because lists are not immutable.

Source: Dive into Python 3


And https://stackoverflow.com/a/1708538/724176:

Apart from tuples being immutable there is also a semantic distinction that should guide their usage. Tuples are heterogeneous data structures (i.e., their entries have different meanings), while lists are homogeneous sequences. Tuples have structure, lists have order.

Using this distinction makes code more explicit and understandable.

One example would be pairs of page and line number to reference locations in a book, e.g.:

my_location = (42, 11)  # page number, line number

You can then use this as a key in a dictionary to store notes on locations. A list on the other hand could be used to store multiple locations. Naturally one might want to add or remove locations from the list, so it makes sense that lists are mutable. On the other hand it doesn't make sense to add or remove items from an existing location - hence tuples are immutable.

There might be situations where you want to change items within an existing location tuple, for example when iterating through the lines of a page. But tuple immutability forces you to create a new location tuple for each new value. This seems inconvenient on the face of it, but using immutable data like this is a cornerstone of value types and functional programming techniques, which can have substantial advantages.

There are some interesting articles on this issue, e.g. "Python Tuples are Not Just Constant Lists" or "Understanding tuples vs. lists in Python". The official Python documentation also mentions this

"Tuples are immutable, and usually contain an heterogeneous sequence ...".


That suggests tuples are better here, and not to use lists. Is there a compelling reason to also accept lists?

@gaoxinge
Copy link
Author

gaoxinge commented Mar 27, 2019

@hugovk I agree with you on that tuple is better than list, and it is the responsibility that user changes the args from list to tuple before call the Pillow function.

However I think there are two compelling reasons to accept list:

  • First, the behaviour that Pillow treats the list args is not self-consistent. We can't pass list to function draw.line, but we can in Image.new, Image.resize or Image.paste.

  • Second, the point args may from np.array or np.array.tolist before call the Pillow function, so it may be better that Pillow function treats the point args as containers with two elements which is "broader" than the tuple with two elements.

@radarhere
Copy link
Member

radarhere commented Jun 30, 2019

@hugovk while you're in the mindset of reviewing things, are you interested in making a final call for this? I don't have strong feelings either way.

@hugovk
Copy link
Member

hugovk commented Jul 1, 2019

Hmm, I also don't have strong feelings either way. It's adding a bunch of conversion code, which I doubt would have much of a performance hit, but means we should try and be consistent everywhere. 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.

@gaoxinge
Copy link
Author

gaoxinge commented Jul 1, 2019

@hugovk @radarhere Can we modify the PyPath_Flatten directly? And Modify the codes below:

Pillow/src/path.c

Lines 177 to 180 in 7585136

else if (PyArg_ParseTuple(op, "dd", &x, &y)) {
xy[j++] = x;
xy[j++] = y;
} else {

Pillow/src/path.c

Lines 195 to 198 in 7585136

else if (PyArg_ParseTuple(op, "dd", &x, &y)) {
xy[j++] = x;
xy[j++] = y;
} else {

Pillow/src/path.c

Lines 224 to 227 in 7585136

else if (PyArg_ParseTuple(op, "dd", &x, &y)) {
xy[j++] = x;
xy[j++] = y;
} else {

I think this way may

  • not need to write too much codes to adapt every kind of coordinates, such as list and tuple
  • not hit the performance too much

@gaoxinge
Copy link
Author

gaoxinge commented Jul 5, 2019

@hugovk @radarhere I have put a pr #3951. You can review and check that is there any problem with it?

@radarhere
Copy link
Member

Thanks for your enthusiasm @gaoxinge, but it sounds like we're leaving it as is.

@Julian-O
Copy link

Hi,

I was kindly linked here after raising #6282.

I appreciate this "what should we accept as parameters" is an old issue, and I am reluctant to re-open it, but I believe it deserves another look. I think the number of issues continuing to being raised around this topic demonstrate this hasn't been resolved yet.

@hugovk wrote above (27 Mar 2019) about the differences between lists and tuples - particularly mutability, performance, and structure versus order.

I understand and agree with the distinctions being made; they are important ones. Where Pillow is storing and processing coordinates, this makes a strong argument that they should be stored as immutable tuples.

However, the right types for storage isn't the question being asked by these issues. Instead, the question being asked is "What types should be accepted via the Pillow API?"

There is a standard answer to the question about what should be accepted via an API, and that answer underlies the design of the Python language. The answer is "Use duck-typing" - to give the most flexibility to the API client, you should accept any value whose type that looks and quacks like the type you need.

When creating an new image, Pillow needs a size parameter. It doesn't need the size parameter to be an immutable, well-structured, performant 2-tuple of integers. It needs the size parameter to support the __iter__ protocol and for it to supply two values that will let themselves be converted to an int.

#3738 is about a client wanting to provide a coordinate as a list instead of a tuple. That is fine, and should be acceptable.

#6282 is about (my) client wanting to provide a class instance that supports all the necessary operations as a coordinate instead of a tuple. That is fine, and should be acceptable.

It is worth noting that different parts of the interface currently treat this differently.

  • draw.line() insists that coordinates are tuples.
  • Image.new() insists that coordinates are tuples or lists.
  • Color3DLUT.__init__() doesn't care about the actual type. It just cares that color table sizes can be split into three, and are each value can be converted to an int.

I think the latter is the right approach†, and should be the direction that Pillow heads for all of the issues about what may be passed as a parameter. Any call to isinstance() outside of a comparison operation should be treated as code smell to be considered carefully.

† (except Color3DLUT then converts the size parameter into a list (!!) for storage. It should be a tuple instead for the reasons provided by hugovk.)

I know you made a decision about this in 2019 and it may be easier just to consider the topic closed, but I still invite you to reconsider. Thank you.

@radarhere
Copy link
Member

A list of lists has now been enabled in #8800

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants