pocketken / h3.net Goto Github PK

Implement polyfill modes:

• index center point-in-poly (current implementation)
• index boundary geometry intersects poly (any vertex point-in-poly)
• index boundary geometry fully contained by poly (all vertices in poly)

Any others?

Edit: also look at whether or not it makes sense to use the NG geometry overlay, and adding test cases for current open upstream polyfill bugs to see if we're good or if there's anything that needs to be fixed for the next release.

Hello @pocketken,

First, say thank you very much for the effort to make this port to C#. We are starting a project and we would like to test H3.

Do you have any plans to implement a conversion from h3 to polygons (something similar to https://github.com/uber/geojson2h3 or close).

Thanks!

Might as well..

I did some tests, comparing the geohash calculated by the official Uber lib vs H3.Net, and the results are completely different

For example:
This lat/long: [-23.553301290491326, -46.65526874921591] on resolution 9

Expected result: 89a8100c187ffff
Result from H3.Net: 890326a6c73ffff

Before the next release, need to:

• clean up some of the duplication introduced in #43; in particular explore the idea of using Coordinate instead of GeoCoord for all the spherical coordinates
• properly mark things internal / public -- right now pretty much everything is public, is that what we want?
• add some API documentation + examples; the unit tests are ok but not great at providing a library overview and (more importantly) how upstream concepts are mapped/implemented in this port. A simple getting started doc with some generated API docs would go a long way
• review the names and concepts RFC and figure out which are applicable
• update benchmarks

Given we have some interest in using the library w/Unity (#48), add netstandard2.0 target and stuff.

There are two quick and easy changes that can be made to H3.Algorithms.Rings.GetKRingSlow to improve performance and reduce allocations:

• we shouldn't be including Direction.Center when iterating neighbouring cells; it just returns itself and we've already emitted that cell (unnecessary allocation and containment check)
• use HashSet<ulong> instead of Dictionary<ulong, int> -- the queue ensures we iterate cells for each k in order, so we won't ever see the same cell at a lower k value like a recursive or stack based implementation would. This means the additional storage of the "last seen" k value is unnecessary and allocating more than we need to

Before:

After:

DefaultGeometryFactory is using EPSG 4236, whereas it should be using 4326.

Investigate implementing the equivalent of upstream PR 562 ('low52' / cell canonicalization) once it looks stable-ish.

Looks like 4.0.0-rc1 is out upstream, so, should probably take some time to look at what documentation and method names need to be updated given we're likely to see a 4.0.0 release in the nearish future.

For next release, need to implement equivalent changes to upstream PR 652 and change Vec2d.Intersect to use double instead of float when calculating t. Will require syncing the changes to the test data, as the increase in precision leads to different geographic coordinates for some cells.

Just trying to get some of the library working, but running into some issues.
In particular, at the moment is finding the vertices of a H3 cell.
I'm trying to use

var vertices = index.GetCellBoundary().Coordinates

but vertices is of length 0. :(
if I use

var vertices = index.GetCellBoundaryVertices().ToArray();

that doesn't seem to be returning anything useful.
It'd be great to have some examples of simple stuff like that, and/or some sort of comparison between this api and the "standard" api.

TIA

Should probably try and ensure that all of the things that have tests in the upstream lib are tested here as well to ensure we're matching upstream behaviour.

• support filling of GeometryCollections (meaning any of POINT, LINESTRING, POLYGON, MULTIPOINT, MULTILINESTRING, MULTIPOLYGON)
• look at whether or not it makes sense to use the NG geometry overlay. Initial quick stab at it shows that it breaks some of the existing polyfill tests, so dragons likely ahead
• add test cases for current open upstream/elsewhere polyfill bugs to see if we're good or if there's anything that needs to be fixed
  • upstream issue 521
  • h3-pg issue 56
• support for different CRS's? The upstream polyfill modes RFC PR makes specific mention of web mercator (3857), however I imagine there's use cases for others as well (maybe 4978?). This should be trivial to support using ProjNet, however, we don't currently expose any way of transforming vertex coordinates prior to containment checks (we're always using the WGS84 lon/lats produced by GeoCoord). This also means anyone trying to use .Fill() right now on a polygon that is not in WGS84 is getting incorrect results.
  • also need to fix the polygon split functionality to support non-WGS84 (or perhaps just don't worry about splitting it if a transform is provided..? or maybe we just expose SplitGeometry as a utility extension and leave it up to the caller to split if using transmeridian geometry..?)
• maybe we should also provide an overload that takes a delegate? That way the user is free to use whatever predicate(s) they want in order to satisfy inclusion of a particular index boundary, e.g. something like Func<Geometry, Geometry, bool> would allow you to do (polygonBeingFilled, cellBoundaryToTest) => polygonBeingFilled.Crosses(cellBoundaryToTest) or .Intersects() and so on. Maybe pass the index being tested as well?

Whatever we do to support CRS transforms in Fill should be extended to any of the other geometry-producing methods as well for consistency.

• add apache license etc.
• documentation
• package name?
• package version should probably match upstream lib? or no?
• publish under pocketken, cenozon, zardozspeakstoyou inc..?
• probably want to use Github Actions to do CI?

Ok, falling a bit behind it looks like so need to get off my behind and show this project a bit of TLC.

So, here's what's changed since 4.0.0 -- will need to spend a bit looking to see which affect us and which don't.

4.0.1

• Changing an internal float to double improves the precision of geographic coordinate output (this is #86)
• Fixed compacting all children of a resolution 0 cell
• Fixed possible signed integer overflow in maxGridDiskSize
• Fixed possible use of uninitialized values in cellToVertex
• Fixed possible out of bounds read in localIjToCell
• Fixed possible memory leak in compactCells
• Fixed possible out of bounds read in areNeighborCells
• Fixed possible memory leak in cellsToLinkedMultiPolygon

4.1.0

Added

• Functions for cellToChildPos and childPosToCell

Fixed

• Fixed possible signed integer overflow in h3NeighborRotations
• Fixed possible signed integer overflow in localIjToCell

Apologies I am new to geospatial processing and I am assuming that H3 Grid Identification should be consistent across libraries. Specifically, when passing a latlng to the H3Index.FromLatLng(latlng, 11); the output grid, when processed via other H3 libraries python etc comes back with a very different Lat lon. This was discovered as I have a large volume of data matched to H3 via other libraries and my whilst processing incoming lat lons via my api I noticed zero collisions. example points below:
lat of 51.524742 and lon of -0.081056
yield 8b0731931c96fff via H3Index above.

python method below -
Define the H3 grid cell
h3_cell = '8b0731931c96fff'

Convert the H3 cell to latitude and longitude coordinates
lat, lon = h3.h3_to_geo(h3_cell)

Print the latitude and longitude
print(f"{lat}, {lon}")
72.15039941409432, -4.643722847528908

Use NetTopologySuite 2.x, not 1.x.

Description

H3Index instances are unable to be serialized to JSON using System.Text.Json, with JsonSerializer.Serialize(someIndex) resulting in an exception:

System.Text.Json.JsonException : A possible object cycle was detected. This can either be due to a cycle or if the object depth is larger than the maximum allowed depth of 64. Consider using ReferenceHandler.Preserve on JsonSerializerOptions to support cycles.

Steps to Reproduce

1. Create a H3Index
2. Attempt to serialize it using JsonSerializer.Serialize()
3. Observe stack trace

Expected Result

We should be serializing to/from the hex string representation of the index.

Hello @pocketken ,

Just wondering if you have considered using struct instead of a sealed class for H3Index to avoid memory allocations on the heap and make better use of processor caching?

I've been considering making a PR but you may already have investigated the subject?

Thanks

EdgeLengthMeters method is always returning 0

var latitude = -23.553301290491326;
var longitude = -46.65526874921591;
var coordinates = new LatLng(latitude , longitude );
var indexH3 = H3Index.FromLatLng(coordinates , 9);
var length = indexH3 .EdgeLengthMeters();

here, the length == 0

Need to address the following and push out a 3.7.2 release:

• need to add the fix and matching tests for h3NeighborRotations (H3HierarchyExtensions.GetDirectNeighbour) from upstream release 3.7.2
• might as well add upstream PR 496, because bitshifting!!1!
• need to fix the JSON serializer so it isn't greedy deserializing strings that aren't indexes

Add netstandard2.1 target and tests / benchmarks for netcoreapp3.1.

Probably also need to port the following:

• line - Given two H3 indexes, return the line of indexes between them (inclusive)
• compact - Compacts the given array as best as possible
• uncompact - Uncompacts the given array at the given resolution. If no resolution is given, then it is chosen as one higher than the highest resolution in the set
• polyfill - Takes an exterior polygon [and a set of hole polygon] and returns the set of hexagons that best fit the structure
• set_to_multi_polygon - Create a LinkedGeoPolygon describing the outline(s) of a set of hexagons. Polygon outlines will follow GeoJSON MultiPolygon order: Each polygon will have one outer loop, which is first in the list, followed by any holes
• indexes_are_neighbors - Returns true if the given indices are neighbors
• maybe need the edge index stuff as well?

After using LineTo and Fill a bit with some real data, I've noticed that these methods slow down quite a bit depending on the resolution being used and the distance/size of the geometry that we're attempting to generate cells for. e.g. the following shows the difference between resolution 10 and 14 for the Uber SF test polygon:

Some of this is just going to come down to the fact that we're generating a lot of indexes, but, there's also a lot of conversion to/from/between H3Index and its corresponding center coordinate for the polyfill stuff, and to/from local IJK for the line. Need to spend some time looking at optimizing and reducing allocations along those code paths to see if there's an opportunity to improve performance when using higher resolutions over larger areas.

I can't remember why exactly I started putting the version into the assembly name, but, it really shouldn't be there as it messes with nuget's ability to only keep the latest version of an assembly in a build. This means in places where we might end up seeing the same assembly with different versions (i.e., benchmarks comparing subsequent releases) you'll likely end up getting the older version of the assembly instead of the newer one.

Kind of hard to do comparative benchmarks when it uses the same DLL for each run..

I've run into problem where seemingly valid index created with FromPoint-method is deemed invalid by the IsValid.

var point = new Point(24.57011413572222, 60.191627502416665) { SRID = 4326 };
var h3Index = H3Index.FromPoint(point, 15);
var index = h3Index.IsValid; // => false

The problem exists only on version 4.0.0, same point is returned as "valid" index by the previous version 3.7.2.1.

The validation seems to fail at last line where leading zeros are checked.

.NET 5 is no longer supported, so at some point should probably stop producing outputs for it.

Setup GH actions to build and publish packages to nuget.org if/when a new version is tagged.