twhiteaker / CFGeom

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

Python "loads" function for simple geometries #6

Closed bekozi closed 7 years ago

bekozi commented 8 years ago

Add function to load a Shapely geometry from index and coordinate arrays. This may be used to compare the Shapely/OGR WKT from the loaded geometry with the test fixture WKT.

bekozi commented 8 years ago

First cut on this is ready for review. All geometries in the simple WKT fixture are handled (holes included!).

BobSimons commented 8 years ago

Sorry for being so dense: Can you please expand the description in the "Important Elements and Structure" section of https://github.com/bekozi/netCDF-CF-simple-geometry so that it explains how the indexing system works? Someone (I) could read the code and figure it out, but it seems like the information needs to be written into the description.

Thank you.

On Wed, Jun 1, 2016 at 12:58 PM, Ben Koziol notifications@github.com wrote:

First cut on this is ready for review. All geometries in the simple WKT fixture are handled (holes included!).

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bekozi/netCDF-CF-simple-geometry/issues/6#issuecomment-223106800, or mute the thread https://github.com/notifications/unsubscribe/ABarOCLqy7k5kknmiy7Q4jfK_BxzIN8Vks5qHeRngaJpZM4Ir5dq .

Sincerely,

Bob Simons IT Specialist Environmental Research Division NOAA Southwest Fisheries Science Center 99 Pacific St., Suite 255A (New!) Monterey, CA 93940 (New!) Phone: (831)333-9878 (New!) Fax: (831)648-8440 Email: bob.simons@noaa.gov

The contents of this message are mine personally and do not necessarily reflect any position of the Government or the National Oceanic and Atmospheric Administration. <>< <>< <>< <>< <>< <>< <>< <>< <><

twhiteaker commented 8 years ago

Also related to the README, why would index_start_stop be 2 or 3 instead of 1 (start) or 2 (start and stop)? I admit I haven't looked at the code yet but am about to.

twhiteaker commented 8 years ago

I think https://github.com/bekozi/netCDF-CF-simple-geometry/blob/master/src/python/ncsg/cf.py#L138 is not doing what you intended. For to_split like this: [0, 1, 2, 3, NCSG_MULTIPART_BREAK_VALUE, 4, 5, 6, 7, 8, 9, NCSG_MULTIPART_BREAK_VALUE, 10, 11, 12, 13] I think you'll get back: [[0, 1, 2, 3], [9], [10, 11, 12, 13]] when what I think you intended to get back is: [[0, 1, 2, 3], [4, 5, 6, 7, 8, 9], [10, 11, 12, 13]]

I'll make changes to produce the latter and make a pull request and @bekozi can decide if he agrees.

bekozi commented 8 years ago

Can you please expand the description in the "Important Elements and Structure" section of https://github.com/bekozi/netCDF-CF-simple-geometry so that it explains how the indexing system works?

@BobSimons I added a ticket for this: https://github.com/bekozi/netCDF-CF-simple-geometry/issues/11. It definitely needs to be done.

Also related to the README, why would index_start_stop be 2 or 3 instead of 1 (start) or 2 (start and stop)? I admit I haven't looked at the code yet but am about to.

@twhiteaker My thought is that start_index or whatever we want to call it should be only 0 or 1. Similar to UGRID convention, the intention of the parameter is to allow C and Fortran-based indexing in the coordinate index array. I'm not sure what the intention of index_start_stop is...

twhiteaker commented 8 years ago

@bekozi I was referring to index_start_stop as mentioned in README.md in this line:

index_start_stop would be 2 or 3 depending if just the first node of a given time series or both the first and last node of a given timeseries is to be stored

bekozi commented 8 years ago

Got it. I'm not sure how this applies to geometries which do not care about time in the basic case (i.e. one could store state boundaries with no temporal information). I think we need to rework the README (as @BobSimons requested) to show how the indexing implementation for the geometries may be inherited by the timeSeries feature type. @dblodgett-usgs and I discussed this briefly at the CF workshop. At this point, I am of the opinion that timeSeries would inherit from the simple geometries specification and not the other way around.

This does bring up the question of repeating the last node with polygons. Most GIS frameworks don't care, but software like ESMF does. I created an issue for this: https://github.com/bekozi/netCDF-CF-simple-geometry/issues/12.

twhiteaker commented 7 years ago

In Python we can read geometries from netCDF and store as Python objects, As of 1592735e630d26a2280abc88e52c399ea89ee394, there is a to_shapely function at https://github.com/twhiteaker/netCDF-CF-simple-geometry/blob/master/src/python/ncsg/cf.py#L96. This function loads shapely geometries from our Python objects. Note that we've dropped index arrays in favor of node counts within netCDF, so we don't use index arrays as initially stated in this issue.