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

Add support for Polygons with holes #3092

Merged
merged 37 commits into from Oct 23, 2018

Conversation

Projects
None yet
3 participants
@philippjfr
Copy link
Contributor

philippjfr commented Oct 19, 2018

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.

  • A MultiInterface can hold a list 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.
  • Each item in a MultiInterface list represents an individual geometry, where a geometry can be a LinearString, LinearRing, Polygon, MultiLineString or MultiPolygon.
  • If an item in the list represents a MultiLineString or MultiPolygon then each polygon/line string should be separated by nans.
  • In order to store holes for MultiPolygons the holes are deeply nested to unambiguously assign each hole to each polygon:
    • 1st. The first level of nesting corresponds to the list of geometries
    • 2nd. The second level corresponds to each Polygon in a MultiPolygon
    • 3rd. The third level of nesting allows for multiple holes per Polygon

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:

coords = [(1, 2), (2, 0), (3, 7)]
holes = [[[(1.5, 2), (2, 3), (1.6, 1.6)], [(2.1, 4.5), (2.5, 5), (2.3, 3.5)]]]
hv.Polygons([{('x', 'y'): coords, 'holes': holes}])

bokeh_plot

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:

coords = [(1, 2), (2, 0), (3, 7), (np.nan, np.nan), (6, 7), (7, 5), (3, 2)]
holes = [[[(1.5, 2), (2, 3), (1.6, 1.6)], [(2.1, 4.5), (2.5, 5), (2.3, 3.5)]], []]
hv.Polygons([{('x', 'y'): coords, 'holes': holes}])

bokeh_plot

To further illustrate this, we can split the MultiPolygon into two separate Polygons like this:

coords = [(1, 2), (2, 0), (3, 7)]
holes = [[[(1.5, 2), (2, 3), (1.6, 1.6)], [(2.1, 4.5), (2.5, 5), (2.3, 3.5)]]]
coords2 = [(6, 7), (7, 5), (3, 2)]
hv.Polygons([{('x', 'y'): coords, 'holes': holes, 'value': 1},
             {('x', 'y'): coords2, 'value': 2}], vdims='value')

bokeh_plot

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.

@jsignell

This comment has been minimized.

Copy link
Collaborator

jsignell commented Oct 19, 2018

Are you losing the mapping from polygon (section of multipolygon) to hole in that transformation?

@jsignell

This comment has been minimized.

Copy link
Collaborator

jsignell commented Oct 19, 2018

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

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Oct 19, 2018

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.

@jsignell

This comment has been minimized.

Copy link
Collaborator

jsignell commented Oct 19, 2018

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?

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Oct 19, 2018

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.

@philippjfr philippjfr referenced this pull request Oct 21, 2018

Merged

Completely rewrote geometry handling #244

7 of 7 tasks complete
@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Oct 21, 2018

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.

@philippjfr philippjfr added the API label Oct 22, 2018

for i, sd in enumerate(d):
unpacked.append((sd, vals[:, i]))

unpacked = []

This comment has been minimized.

@philippjfr

philippjfr Oct 22, 2018

Author Contributor

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():

This comment has been minimized.

@philippjfr

philippjfr Oct 22, 2018

Author Contributor

A column of NaNs was not being detected as being scalar.

]
}
],
"metadata": {

This comment has been minimized.

@jlstevens

jlstevens Oct 22, 2018

Contributor

Remember to clear out this metadata before this PR can be merged...

This comment has been minimized.

@philippjfr

philippjfr Oct 22, 2018

Author Contributor

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:"

This comment has been minimized.

@jlstevens

jlstevens Oct 22, 2018

Contributor

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.

This comment has been minimized.

@philippjfr

philippjfr Oct 22, 2018

Author Contributor

Okay removing this.

"cell_type": "markdown",
"metadata": {},
"source": [
"If a polygon has no holes at all the 'holes' key may be ommitted entirely:"

This comment has been minimized.

@jlstevens

jlstevens Oct 22, 2018

Contributor

Maybe worth doing this first (no holes) before introducing holes? I.e a reminded of the Polygon constructor before holes were supported...

This comment has been minimized.

@philippjfr

philippjfr Oct 22, 2018

Author Contributor

Sure.

This comment has been minimized.

@philippjfr

philippjfr Oct 22, 2018

Author Contributor

Actually I think saying that the format is exactly the same as the paths one is enough, otherwise it gets a bit repetitive.

This comment has been minimized.

@jlstevens

jlstevens Oct 22, 2018

Contributor

Sure.

" {'x': [4, 6, 6], 'y': [0, 2, 1], 'value': 1}\n",
"], vdims='value')\n",
"\n",
"polys = poly.split()\n",

This comment has been minimized.

@jlstevens

jlstevens Oct 22, 2018

Contributor

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.

This comment has been minimized.

@philippjfr

philippjfr Oct 22, 2018

Author Contributor

Sounds good.

@@ -185,3 +186,33 @@ def get_raster_array(image):
else:
data = np.flipud(data)
return data

def ring_coding(array):

This comment has been minimized.

@jlstevens

jlstevens Oct 22, 2018

Contributor

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...

This comment has been minimized.

@philippjfr

philippjfr Oct 22, 2018

Author Contributor

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:

This comment has been minimized.

@jlstevens

jlstevens Oct 22, 2018

Contributor

Why can't you use the super definition of holes if holes aren't defined/supported?

This comment has been minimized.

@jlstevens

jlstevens Oct 22, 2018

Contributor

Unless the default implementation works for all interfaces except this one specifically?

This comment has been minimized.

@philippjfr

philippjfr Oct 22, 2018

Author Contributor

Probably can do that.

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Oct 22, 2018

I've made a few comments but overall this PR looks good. This feature has been a long time coming!

@philippjfr philippjfr added this to the v1.11.0 milestone Oct 22, 2018

Philipp Rudiger added some commits Oct 22, 2018

Philipp Rudiger Philipp Rudiger
Philipp Rudiger Philipp Rudiger
Philipp Rudiger Philipp Rudiger
Philipp Rudiger Philipp Rudiger
Philipp Rudiger Philipp Rudiger

@philippjfr philippjfr force-pushed the poly_holes branch from 38e5569 to ff319a6 Oct 22, 2018

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Oct 23, 2018

The tests have now passed (finally!). All looks good to me.

@jlstevens jlstevens merged commit cfb4b62 into master Oct 23, 2018

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.3%) to 88.889%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr referenced this pull request Oct 23, 2018

Closed

Improve contours, fix fill colors for contours #1558

5 of 5 tasks complete

@philippjfr philippjfr deleted the poly_holes branch Nov 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.