drufat / triangle

Python bindings to the triangle library
GNU Lesser General Public License v3.0
230 stars 53 forks source link

Issue with new way triangle.triangulate operates #31

Closed cedh4c closed 5 years ago

cedh4c commented 5 years ago

First of all, thank you for this very useful package. I was working with the 2017 release until now without problems and today tried on a new install that fetched the 2019 release. I found out the hard way that triangle.triangulate has drastically changed the way in handles the input dict and it doesn't allow unexpected keys anymore. I'm probably the only one, but I find it handy to be able to bundle additional data in the dict under extra keys. The 2017 version didn't care because it had a preset list of keys in which it would go through and just ignore the other ones (but still return the final dict with the extra keys). In the 2019, you iterate through all the keys in the dict and try to match all of them with the fwd translation and reverse translation. That breaks if the dict contains any unexpected key. Would you mind changing this slightly so that extra keys can be passed through? I could do it and submit a PR if you want. It shouldn't be much more than a few lines, I think. Obviously, the user can filter the dict before feeding to triangle but I think allowing pass-through of extra keys would make the package more flexible. Let me know what you think.

drufat commented 5 years ago

You are correct. The new version of triangle only allows dictionary keys that it understands, and does not let any "pass through" data. Generally I think it is good practice to let functions only accept input data that is relevant to them.

I would recommend that in your own code you create a wrapper function around triangle.triangulate that has the desired behavior. I would be open to merging a pull request as long as it does not add too much complexity to the code. The python wrapper uses slightly different argument names than the the underlying C library, so the wrapper has to translate back and forth between those names:

terms = (
    ('pointlist', 'vertices'),
    ('pointattributelist', 'vertex_attributes'),
    ('pointmarkerlist', 'vertex_markers'),

    ('trianglelist', 'triangles'),
    ('trianglearealist', 'triangle_max_area'),

    ('segmentlist', 'segments'),
    ('segmentmarkerlist', 'segment_markers'),

    ('holelist', 'holes'),

    ('regionlist', 'regions'),
)

I wanted to make this correpsondence explicit in the new version, and that's why the code is now more strict about what's in the input dictionary.

cedh4c commented 5 years ago

I can see your point about relevance of input data. I wasn't looking at it that way, but it definitely makes sense. I think you're right. Probably better that I wrap on my side and leave triangle clean. It establishes a clear line and we only have to agree on the expected inputs/outputs. Thanks for the fast response!

cedh4c commented 5 years ago

By the way, wrapping on my side was really easy and took only 3 lines... I was making a fuss for nothing. Thank you again for your work on this package, it is super helpful.