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 H3 layer #198

Merged
merged 7 commits into from Aug 27, 2018

Conversation

Projects
None yet
4 participants
@ericsoco
Collaborator

ericsoco commented Aug 14, 2018

Still planning to add a coverage slider to match existing Hexbin Layer.
Also, I think @nrabinowitz has some additional edits.

This addresses #154, but does not add support for H3 bin aggregation (that can come in a followup diff, extending our use of h3-js.

screen shot 2018-08-14 at 11 20 27 am

ericsoco added some commits Aug 13, 2018

@ericsoco ericsoco changed the title from Add H3 layer (WIP) to Add H3 layer Aug 14, 2018

@ericsoco

This comment has been minimized.

Collaborator

ericsoco commented Aug 14, 2018

Still TODO:

  • Coverage slider
  • @nrabinowitz additional h3-utils refinements
  • Correctly populate + render "Layer Type" UI in H3 layer panel
  • Figure out why CI is failing (tests pass locally...)
  • H3 bin aggregation: probably want to implement as switch and conditional slider in Hexbin: "use H3 hexes" and, when that's flipped on, "H3 resolution" (instead of "Hexagon Radius (km)"). Most likely will save this for a future PR.
  • Correctly serialize H3 settings, for config export
  • Don't auto-add point layer when H3 can be auto-detected
@heshan0131

This comment has been minimized.

Member

heshan0131 commented Aug 15, 2018

This is awesome. Thank you for doing this. Did you move the files from @uber/kepler.gl?

@@ -3506,6 +3506,10 @@ grid-index@^1.0.0:
version "1.0.0"
resolved "https://registry.yarnpkg.com/grid-index/-/grid-index-1.0.0.tgz#ad2c5d54ce5b35437faff1d70a9aeb3d1d261110"
h3-js@^3.1.0:

This comment has been minimized.

@heshan0131

heshan0131 Aug 16, 2018

Member

Please update your ~/.npmrc to use registry = https://registry.yarnpkg.com/ before running yarn. This is why the build failed

@ericsoco

This comment has been minimized.

Collaborator

ericsoco commented Aug 20, 2018

@heshan0131 yep, copied from internal kepler wrapper. Will try to find time this week to wrap it up, TBD.

@ibgreen

Nice work! Is this generic enough that we should we consider moving it to deck.gl?

@ericsoco

This comment has been minimized.

Collaborator

ericsoco commented Aug 20, 2018

@ibgreen perhaps? Thoughts on where in deck.gl this would be added, experimental-layers to start I assume? (Been a while since I contributed.)

Would probably want to standardize enhanced-hexagon-cell-layer across this PR and the existing one in Kepler, and may need some other cleanup.

I'm guessing the best path here is to land this into Kepler first and then work w/ @heshan0131 and @nrabinowitz to tidy things up enough to hoist them into deck.gl. Thoughts?

@ibgreen

This comment has been minimized.

ibgreen commented Aug 20, 2018

@ericsoco - All sounds reasonable.

Also see the ideas in the deck.gl layers roadmap where we start discussing having multiple layer catalogs.

Putting things in experimental layers is a nice first start, since more users can access the layers.

The big job is typically getting layers out of experimental layers and into the official catalog(s).

@nrabinowitz

LGTM, just a few perf and cleanup notes.

hexagonResolution: this.dataToFeature.resolution
};
// return {layerData, layer: this};

This comment has been minimized.

@nrabinowitz

nrabinowitz Aug 21, 2018

Collaborator

Kill comment please

getElevation,
getColor,
getHexId,
hexagonVertices: this.dataToFeature.vertices,

This comment has been minimized.

@nrabinowitz

nrabinowitz Aug 21, 2018

Collaborator

Is this still needed? We calculate in render now, right?

allData.forEach((d, index) => {
const id = getHexId(d);
if (typeof id !== 'string' || !id.length) {

This comment has been minimized.

@nrabinowitz

nrabinowitz Aug 21, 2018

Collaborator

Consider h3IsValid here, which checks whether it's a real H3 address. This would be slower but more reliable.

}
renderLayer({
data,

This comment has been minimized.

@nrabinowitz

nrabinowitz Aug 21, 2018

Collaborator

This may be a Kepler.gl style question, but I find the way that data refers to different objects in different methods a bit confusing.

// get vertices should return [lon, lat]
export function getVertices({id}) {
// always reverse it
return h3ToGeoBoundary(id).map(d => d.reverse());

This comment has been minimized.

@nrabinowitz

nrabinowitz Aug 21, 2018

Collaborator

h3ToGeoBoundary(id, true) will give you GeoJSON [lon, lat] coords in a more performant way than .reverse(). It will also close the loop (repeat the first point), which is required for valid GeoJSON - not sure if that matters here.

return {
geometry: {
coordinates: [...vertices, vertices[0]],

This comment has been minimized.

@nrabinowitz

nrabinowitz Aug 21, 2018

Collaborator

No need for vertices[0] here if you use the h3ToGeoBoundary(id, true) suggestion above.

const vertices = getVertices(object);
return {
geometry: {

This comment has been minimized.

@nrabinowitz

nrabinowitz Aug 21, 2018

Collaborator

May not matter here, but this isn't a valid GeoJSON object w/o "type": "Feature" at the top level.

}
// get centroid should return [lon, lat]
export function getCentroid({id}) {

This comment has been minimized.

@nrabinowitz

nrabinowitz Aug 21, 2018

Collaborator

It probably wouldn't matter if it wasn't in a hot loop, but you might be doing this for 100k hexagons, so I'd suggest just passing the hex id w/o needless destructing.

@heshan0131

This comment has been minimized.

Member

heshan0131 commented Aug 27, 2018

@ericsoco I can take over this PR. Please grant me access to your fork. You can allow edits from maintainers

heshan0131 added some commits Aug 27, 2018

@heshan0131 heshan0131 merged commit a8ea76f into uber:master Aug 27, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment