twhiteaker / CFGeom

CF Convention for Representing Simple Geometry Types
MIT License
9 stars 4 forks source link

Rework Wiki Intro to the Specification #54

Closed dblodgett-usgs closed 7 years ago

dblodgett-usgs commented 7 years ago

Building on the general readme overview in #53 - Modify what's there to reflect changes.

dblodgett-usgs commented 7 years ago

Getting going on this, will work on it offline today, hope to have something up by evening.

dblodgett-usgs commented 7 years ago

Alright. https://github.com/twhiteaker/netCDF-CF-simple-geometry/wiki

This brought up a few things about my draft implementation that should get another look.

twhiteaker commented 7 years ago

What's your rationale for part_node_count rather than node_count? I think of a multipoint as a multipart geometry, where each part consists of a single node, so part_node_count would always be an array of ones whose length equals the number of parts in the instance. I think node_count is more appropriate since you really are counting the number of nodes in the multipoint. In my wiki edit, I reworded the multipoint sections to reflect this.

Your CRS idea sounds good.

WKT and GeoJSON distinguish polygons with holes and from multipolygons. A polygon with holes does consist of multiple parts, so to speak, but there is only one exterior ring (in the WKT+GeoJSON definition). The "multi" in multipolygon means you can have more than one exterior ring.

Can you reword the geometry section so that it doesn't use node_count, since that attribute isn't required for point geometries? Something like this (if I got your intent)?

geometry Carried by any 1-d variable whose instances are represented by geometries defined by the geometry container variable named by this attribute. Comparable to the cell_bounds attribute but links to representative geometry for each position in the variable.

Are we sure we want to require anticlockwise outside rings and first-last node equivalence? If so, we should add those checks to our code.

dblodgett-usgs commented 7 years ago

I wasn't thinking about the multipoint right. You are right, each part is of length 1.

The geometry attribute definition is going to be a bit picky to make sure we match the CF spec terminology and are clear about how things relate. I find it ironic that the simplest case is actually the most complicated because it is so degenerate. How is this for the geometry attribute definition?

geometry
Carried by any variable that shares a dimension with the geometry instances defined by the geometry container variable named by this attribute. Comparable to the cell_bounds attribute, but links to representative geometry for each position of dimension of the shared dimension.

I think we should require both anticlockwise and first-last equivalence. I know my code gets these right, but I don't test for it super carefully. The R spatialPolygons object that I coerce data to/from enforces it for me so I'm kind of playing it lose.

twhiteaker commented 7 years ago

Geometry definition looks good.

The Python implementation doesn't enforce those polygon requirements. I'd want to lean on Shapely to determine node order for me. This may mean a major rewrite of the code, using Shapely geometries as our primary object type rather than our custom type. It might actually make the code cleaner, with the downside being a tighter dependency on Shapely and its geometry types, i.e.., no true curves or ellipses in the future. I need to ruminate a little more.

twhiteaker commented 7 years ago

Wiki looks good. Python implementation is a separate issue.