Improve H3#hexRing logic and add H3#areNeighborCells method #91140
Conversation
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
|
Hi @iverase, I've created a changelog YAML for you. |
|
@elasticsearchmachine update branch |
|
@elasticsearchmachine run elasticsearch-ci/bwc |
|
@elasticsearchmachine run elasticsearch-ci/packaging-tests-unix-sample |
| CoordIJK.Direction.K_AXES_DIGIT, | ||
| CoordIJK.Direction.I_AXES_DIGIT }; | ||
|
|
||
| private static final CoordIJK.Direction[] NEIGHBORSETCOUNTERCLOCKWISE = new CoordIJK.Direction[] { |
There was a problem hiding this comment.
I don't quite understand the code where these are used, but cannot help notice that they do not represent the opposite order of each other. I see that JK, IJ, J, IK, K, I is not the reverse of IK, JK, K, IJ, I, J.
There was a problem hiding this comment.
This is coming from here: https://github.com/uber/h3/blob/87de60bb5c88a72b09c7c8dca0c2df4e7ac1ddef/src/h3lib/lib/directedEdge.c#L94
I am afraid if you really wan to understand it, you need to ask the creators of H3
| int neighborResult = h3NeighborRotations(origin, DIRECTIONS[i].digit(), rotations, nextNeighbor); | ||
| if (neighborResult != E_PENTAGON) { | ||
| // E_PENTAGON is an expected case when trying to traverse off of | ||
| long neighbor = h3NeighborRotations(origin, DIRECTIONS[i].digit()); |
There was a problem hiding this comment.
It looks like the original method must have been used in other cases where the rotations had some value. I wonder if we want to change the method name to reflect the new, simpler, behaviour?
There was a problem hiding this comment.
Changed the name to h3NeighborInDirection.
The difference is that the original method allowed to get the k-ring, being k the distance in H3bin of the ring. We only allow to get the first ring so it can be simplified.
| // E_PENTAGON is an expected case when trying to traverse off of | ||
| long neighbor = h3NeighborRotations(origin, DIRECTIONS[i].digit()); | ||
| if (neighbor != -1) { | ||
| // -1 is an expected case when trying to traverse off of |
There was a problem hiding this comment.
I guess this case is actually when iterating around a pentagons neighbours we will need to skip one. Perhaps the comment could be improved?
There was a problem hiding this comment.
That is what that comment says.
| // Child 0 is neighbor with all of its parent's 'offspring', the other | ||
| // children are neighbors with 3 of the 7 children. So a simple comparison | ||
| // of origin and destination parents and then a lookup table of the children | ||
| // is a super-cheap way to possibly determine they are neighbors. |
There was a problem hiding this comment.
This seems to indicate that the h3NeighborRotations method is expensive. I wonder how this will affect our plans to improve the performance of the HexGridTiler by using these methods. Certainly I want to benchmark that,
| // neighbors across the deleted subsequence, they will fail the | ||
| // optimized check below, but they will be accepted by the | ||
| // gridDisk check below that. | ||
| throw new IllegalArgumentException("Undefined error checking for neighbors"); |
There was a problem hiding this comment.
I have to admit the errors thrown in the code are very uninformative. Most of them are empty strings, and this one is almost as uninformative. I guess these all come from the original H3 code.
There was a problem hiding this comment.
I must say I don't know how to improve it. Please feel free to do it.
| */ | ||
| private static int h3NeighborRotations(long origin, int dir, int[] rotations, long[] out) { | ||
| private static long h3NeighborRotations(long origin, int dir) { |
There was a problem hiding this comment.
Since you removed the previous capability of returning the set of rotations used to find the neighbour, I guess we could also consider renaming the method since it no longer does what its name implies?
* main: (1300 commits) update c2id/c2id-server-demo docker image to support ARM (elastic#91144) Allow legacy index settings on legacy indices (elastic#90264) Skip prevoting if single-node discovery (elastic#91255) Chunked encoding for snapshot status API (elastic#90801) Allow different decay values depending on the score function (elastic#91195) Fix handling indexed envelopes crossing the dateline in mvt API (elastic#91105) Ensure cleanups succeed in JoinValidationService (elastic#90601) Add overflow behaviour test for RecyclerBytesStreamOutput (elastic#90638) More actionable error for ancient indices (elastic#91243) Fix APM configuration file delete (elastic#91058) Clean up handshake test class (elastic#90966) Improve H3#hexRing logic and add H3#areNeighborCells method (elastic#91140) Restrict direct use of `ApplicationPrivilege` constructor (elastic#91176) [ML] Allow NLP truncate option to be updated when span is set (elastic#91224) Support multi-intersection for FieldPermissions (elastic#91169) Support intersecting multi-sets of queries with DocumentPermissions (elastic#91151) Ensure TermsEnum action works correctly with API keys (elastic#91170) Fix NPE in auditing authenticationSuccess for non-existing run-as user (elastic#91171) Ensure PKI's delegated_by_realm metadata respect run-as (elastic#91173) [ML] Update API documentation for anomaly score explanation (elastic#91177) ... # Conflicts: # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackClientPlugin.java # x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/downsample/RollupShardIndexer.java # x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/downsample/TransportRollupIndexerAction.java # x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/v2/RollupActionSingleNodeTests.java
Clean up the logic as we are allowing only first neighbours so we can simplify it a bit and remove some unnecessary allocations. In addition we ported the method
H3#areNeighborCellswhich can be useful for example for aggregations over geo_shape.