Skip to content

Should ContourSet.allsegs and .allkinds be deprecated? #26913

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

Closed
rcomer opened this issue Sep 25, 2023 · 6 comments · Fixed by #27088
Closed

Should ContourSet.allsegs and .allkinds be deprecated? #26913

rcomer opened this issue Sep 25, 2023 · 6 comments · Fixed by #27088
Milestone

Comments

@rcomer
Copy link
Member

rcomer commented Sep 25, 2023

... decoding paths is a lot harder than dealing with a list segments. Given that contourpy returns a list of segments I think it would be nice if we made that available to users. It seems that is where we were before the deprecation, so perhaps it should be reconsidered?

Originally posted by @jklymak in #26863 (comment)

@rcomer rcomer changed the title Should ContourSet.allsegs and .allpaths be deprecated? Should ContourSet.allsegs and .allkinds be deprecated? Sep 25, 2023
@jklymak
Copy link
Member

jklymak commented Sep 25, 2023

Or maybe instead of leaving them as properties, they are rewritten as a helper get_allsegs_allkinds()? Again appreciate that some folks will want to access this directly from contourpy, but making this available doesn't seem too crufty to me.

@anntzer
Copy link
Contributor

anntzer commented Sep 25, 2023

Perhaps more general would be to have a Path().split() method (name up to bikeshedding) that takes a compound path (i.e. possibly with internal MOVETOs) and splits it in paths without internal MOVETOs? (i.e. the opposite of Path.make_compound_path)
Then the requested functionality would be something like [(subpath.vertices, subpath.codes) for path in contour.get_paths() for subpath in path.split()].

@jklymak
Copy link
Member

jklymak commented Sep 25, 2023

That seems reasonable, if an implementation detail. The above is still a mouthful for most people.

@anntzer
Copy link
Contributor

anntzer commented Sep 25, 2023

I'm a bit curious, though about why MOVETOs are a problem for you. After all, if you already handle allkinds, that means you already handle CLOSEPOLYs and need to differentiate between open and closed polygons, which seems already more complex than splitting at MOVETOs?

@jklymak
Copy link
Member

jklymak commented Sep 25, 2023

For my application, I didn't need to know if the polygon was closed or not. I was making KML files of the contours (for using in Google Earth) and I just needed the paths. But I needed each path segment on its own so I needed lists of contiguous segments.

Could I figure it out from the Path API? Probably. But it's understandably frustrating when CountourSet is documented to ingests allsegs in exactly the format I want, that it looses that information and I need to parse it out from the Path API.

... or put another way; I don't think 99.9% of our users know the Path API, and I don't think they should need to learn it for this application.

@anntzer
Copy link
Contributor

anntzer commented Sep 26, 2023

I don't feel overly strongly about this; we could just restore the old allsegs/allkinds properties (and fix them to match the old format, as you note in #26863 (comment), and mark them clearly as "exposed for practicality" so that they don't get removed in a future cleanup).

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

Successfully merging a pull request may close this issue.

4 participants