FIX: contourf nested contours #2225
Conversation
| @@ -188,8 +193,8 @@ def path_to_geos(path, force_ccw=False): | |||
| geom_collection = [] | |||
| for external_geom, internal_polys in collection: | |||
| if internal_polys: | |||
| # XXX worry about islands within lakes | |||
There was a problem hiding this comment.
I assume that XXX is the same as TODO and I'm taking a punt that this is now covered.
| if geom.is_empty: | ||
| pass | ||
| elif (len(collection) > 0 and | ||
| isinstance(collection[-1][0], sgeom.Polygon) and | ||
| isinstance(geom, sgeom.Polygon) and | ||
| collection[-1][0].contains(geom.exterior)): | ||
| collection[-1][1].append(geom.exterior) | ||
| if any(internal.contains(geom) for internal in collection[-1][1]): | ||
| collection.append((geom, [])) |
There was a problem hiding this comment.
Although I’ve tested this across quite a few plots, I’m now questioning whether it is quite right. If we append the geom from the third level down to collection, could the next geom be at the second level down and so would need to check against something earlier than collection[-1]?
There was a problem hiding this comment.
Hm... that is a good question. I wonder if you could somehow keep track of the "last" geometry in the nested collections and sort of recursively drill down/out from there?
There was a problem hiding this comment.
I was thinking of this as a tree problem with the path segments ordered in a "depth first" way. However looking at matplotlib/matplotlib#25247, I think the paths that used to be separate are now just concatenated together. So anything at the second level has to be directly preceded by either something else at the second level or something at the first level, and so my above hypothetical can't happen for contours at least.
There was a problem hiding this comment.
Thanks, for the fast fix @rcomer! I think this is an improvement and handles the case you were going for without breaking anything else. If there is a better way to do this I think we can handle that in follow-up PRs since this is an improvement as-is for now and unblocks the release.
| if geom.is_empty: | ||
| pass | ||
| elif (len(collection) > 0 and | ||
| isinstance(collection[-1][0], sgeom.Polygon) and | ||
| isinstance(geom, sgeom.Polygon) and | ||
| collection[-1][0].contains(geom.exterior)): | ||
| collection[-1][1].append(geom.exterior) | ||
| if any(internal.contains(geom) for internal in collection[-1][1]): | ||
| collection.append((geom, [])) |
There was a problem hiding this comment.
Hm... that is a good question. I wonder if you could somehow keep track of the "last" geometry in the nested collections and sort of recursively drill down/out from there?
Rationale
Closes #2224. See #2224 (comment) for reasoning.
I ran my "real work" test against this branch and all the plots look fine.
Implications