-
-
Notifications
You must be signed in to change notification settings - Fork 403
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
Add support for Polygons with holes #3092
Conversation
Are you losing the mapping from polygon (section of multipolygon) to hole in that transformation? |
nevermind I misread. I think that seems like a reasonable representation. It might be worth looking at geopandas though and trying to make that work well |
Geopandas support will follow in geoviews, but there we don't have to worry about the representation since geopandas already represents holes. |
Oh I see, so we won't transform it to this style? |
No, it should generally stay in its native format, i.e. inside shapely objects. |
Slight correction now that I've started on the geoviews representation, in geoviews paths and polygons are converted to lists of dictionaries containing shapely geometries, which simplifies projections massively. |
for i, sd in enumerate(d): | ||
unpacked.append((sd, vals[:, i])) | ||
|
||
unpacked = [] |
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.
Note that I simply unindented this section when I thought I'd have to complicate it further, there are no changes to DictInterface.init
that need reviewing.
unique = set(values) | ||
else: | ||
unique = np.unique(values) | ||
if (~util.isfinite(unique)).all(): |
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.
A column of NaNs was not being detected as being scalar.
] | ||
} | ||
], | ||
"metadata": { |
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.
Remember to clear out this metadata before this PR can be merged...
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.
Will do.
"\n", | ||
"The ``Path`` element represents a collection of path geometries with associated values. Each path geometry may be split into sub-geometries on NaN-values and may be associated with scalar values or array values varying along its length. In analogy to GEOS geometry types a Path is a collection of LineString and MultiLineString geometries with associated values.\n", | ||
"\n", | ||
"While many different formats are accepted in theory, natively HoloViews provides the ``MultiInterface`` which allows representing paths as lists of regular columnar data objects including arrays, dataframes and dictionaries of column arrays and scalars. A simple path geometry may therefore be drawn using:" |
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.
Are interfaces discussed elsewhere in the user guide? If not, I wouldn't mention MultiInterface
explicitly, if so, I might want to point to where interfaces are discussed.
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.
Okay removing this.
"cell_type": "markdown", | ||
"metadata": {}, | ||
"source": [ | ||
"If a polygon has no holes at all the 'holes' key may be ommitted entirely:" |
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.
Maybe worth doing this first (no holes) before introducing holes? I.e a reminded of the Polygon
constructor before holes were supported...
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.
Sure.
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.
Actually I think saying that the format is exactly the same as the paths one is enough, otherwise it gets a bit repetitive.
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.
Sure.
" {'x': [4, 6, 6], 'y': [0, 2, 1], 'value': 1}\n", | ||
"], vdims='value')\n", | ||
"\n", | ||
"polys = poly.split()\n", |
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.
Don't think it is worth introducing the polys
handle here (just inline it to the Layout
). Looks to me like the handle that is reused is just poly
.
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.
Sounds good.
@@ -185,3 +186,33 @@ def get_raster_array(image): | |||
else: | |||
data = np.flipud(data) | |||
return data | |||
|
|||
def ring_coding(array): |
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.
Is this called ring
because it is closed (or at least looks closed)? If so the docstring should mention this, if not, I'm not sure what 'ring' is referring to. If I remember right, matplotlib has explicit codes for declaring genuinely closed paths that I assume you've considered using...
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.
It generates the path codes for each of the exterior and interior paths (which are always rings in a polygon). Using CLOSEPOLY path codes is not necessary since PathPatch will automatically close any patches.
key = Polygons._hole_key | ||
if key in dataset.data: | ||
return [[[np.asarray(h) for h in hs] for hs in dataset.data[key]]] | ||
else: |
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.
Why can't you use the super definition of holes
if holes aren't defined/supported?
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.
Unless the default implementation works for all interfaces except this one specifically?
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.
Probably can do that.
I've made a few comments but overall this PR looks good. This feature has been a long time coming! |
38e5569
to
ff319a6
Compare
The tests have now passed (finally!). All looks good to me. |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
In conjunction with the corresponding geoviews PR this represents a full protocol for round-tripping data between the dictionary format in holoviews and shapely geometries. Therefore this formally defines the conventions of our format relative to GEOS geometry definitions.
MultiInterface
can hold alist
of standard columnar data structures, the columnar data structures may be, numpy arrays, dictionaries of columns, (pandas/dask) dataframes and dictionaries of a geometry and other data.MultiInterface
list
represents an individual geometry, where a geometry can be a LinearString, LinearRing, Polygon, MultiLineString or MultiPolygon.To demonstrate this let's take a few examples of polygon geometries with holes. In the simplest case we have a Polygon of a few coordinates with holes defined as a list of list of arrays:
In the case of a
MultiPolygon
which contains the Polygon from above it becomes clearer. The coords now contain a nan separator and there are two separate lists of holes, one the same as above the second empty:To further illustrate this, we can split the
MultiPolygon
into two separate Polygons like this:producing almost the same plot but allowing us to set two distinct values for each polygon, when in the MultiPolygon case they would have had to share the same value.
This scheme can also be used to faithfully round-trip matplotlib geometries (e.g. Patches) to this format, which means we can finally handle the
contours
operation correctly.