Skip to content
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

Polyfill modes RFC #519

Closed
wants to merge 6 commits into from
Closed

Conversation

isaacbrodsky
Copy link
Collaborator

No description provided.

@coveralls
Copy link

coveralls commented Oct 2, 2021

Coverage Status

Coverage remained the same at 98.2% when pulling 4f5666c on isaacbrodsky:polyfill-modes-rfc into b501c53 on uber:master.

dev-docs/RFCs/v4.0.0/polyfill-modes-rfc.md Outdated Show resolved Hide resolved
dev-docs/RFCs/v4.0.0/polyfill-modes-rfc.md Outdated Show resolved Hide resolved
dev-docs/RFCs/v4.0.0/polyfill-modes-rfc.md Outdated Show resolved Hide resolved
consistency with the rest of the library, the `polyfill` functions should be able to use the same
cell boundaries.

* Very large polygons
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have suggestions here? E.g. maxPolyfillSizes could return an array of sizes, and polyfillIncremental(batchNum, polygon, out) could polyfill a batch? (How this would work in the lib is still a bit fuzzy, but I think it's possible)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was considering if it would be possible to do this in an iterator but haven't looked into this more. We would need some allocation done, but perhaps that could be made relative to the number of points in the polygon rather than the number of cells. A simple polygon producing a large number of cells is related to the problem seen in zachasme/h3-pg#56

* Spherical geometry consistency

Most of the H3 library uses spherical geoemtry. For example, cell boundaries are spherical hexagons
and pentagons. The `polyfill` function is different that it assumes Cartesian geometry. For
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this RFC refer to polyfill or polygonToCells?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm inconsistent in the term used, but we should do a pass making that consistent. polyfill might be useful as a term as the abstract concept of filling a polygon (regardless of inclusion mode, etc).

## Approaches

*What are the various options to address this issue?*

On an API level, we will need to expose the containment mode and spherical/Cartesian choice as
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we might decide in v4 that polyfill is spherical, full stop, unless it's much slower this way. But I'm not sure there's a benefit to exposing both modes to the user, and it would mean a bunch of redundant code to check point inclusion and line intersection in both ways.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it was raised offline that cartesian is useful when maps are drawn on top of web mercator maps.

Isaac Brodsky and others added 3 commits November 30, 2021 16:06
Co-authored-by: Nick Rabinowitz <public@nickrabinowitz.com>

```js
polygonToCells(polygon, {res: res, cartesian: true, containment: h3.Containment.CENTROID})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: In JS, if a new object's property exactly matches a variable that will fill in that property, you can just list the property/variable name and it'll do the right thing. Eg:

Suggested change
polygonToCells(polygon, {res: res, cartesian: true, containment: h3.Containment.CENTROID})
polygonToCells(polygon, {res, cartesian: true, containment: h3.Containment.CENTROID})

| Bits | Meaning
| ---------- | -------
| 1-2 (LSB) | If 0, containment mode centroid.<br>If 1, containment mode cover.<br>If 2, containment mode intersects.<br>3 is a reserved value.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd reserve more bits here, there are other possible approaches (e.g. > 50% containment). Got plenty of bits.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in make the flags 64 bits?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, as in make this bit section wider (say 3 bits instead of two) so that we don't constrain ourselves to 4 containment modes.

@isaacbrodsky isaacbrodsky marked this pull request as ready for review December 17, 2021 00:07
@isaacbrodsky isaacbrodsky changed the title Draft: Polyfill modes RFC Polyfill modes RFC Dec 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants