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

Support separated (struct) coordinates for all applicable layers #139

Merged
merged 6 commits into from
Feb 11, 2025

Conversation

gmoney1729
Copy link
Contributor

@gmoney1729 gmoney1729 commented Jan 28, 2025

Addresses the issue discussed in #137. (Closes #137)

Overview of the proposed changes:

  1. Helper functions in utils to create linestring or polygon data with interleaved (FixedSizeList) coordinates from linestring or polygon data with separated (Struct) coordinates.
  2. Wherever we work with the polygon data in solid-polygon-layer, we create interleaved polygon data if the underlying coordinates are arranged in a Struct.

Thoughts:

  1. I was first considering changing the getPointChild function under child.ts in geoarrow repo to create interleaved data on the fly but the earcutSinglePolygon call from earcut.ts gets quite expensive because it first gets all the coordinates of all the polygons in the polygon data and then subarrays for the single polygon at the given index of interest.
  2. Thus, I decided to transform the polygon data at the layer level and let earcut continue to work the way it does.

To do:

  1. If this approach makes sense, I can look to other layers as well.
  2. Perform additional checks in utils under isDataInterleavedCoords and isDataSeparatedCoords data (as mentioned by Kyle) such as:
    • 2, 3 or 4d data
    • names of the fields in a struct array
    • data type of the coordinates (float)
  3. Extend PointData type to consider higher dimensions under type.ts in geoarrow repo.

@kylebarron
Copy link
Member

This looks really great! Thanks!

I agree that the simplest and best method for now is to transform the separated coordinates to interleaved coordinates while we're validating them.

If you could extend this to other layers, that would be great, and then we can merge.

@gmoney1729
Copy link
Contributor Author

Thanks for looking into it and putting this suite of tools together in the first place. Appreciated!

Following layers have been updated:

  1. Arc
  2. Column
  3. Heatmap
  4. Path
  5. Point Cloud
  6. Scatterplot
  7. Solid Polygon (done initially to verify approach)
  8. Text
  9. Trips

Changes to the following layers were not applicable and thus left untouched:

  1. H3-Hexagon (not much familiar with this type but looks like it uses grid, so no float coordinates)
  2. Polygon Layer (Uses Solid Polygon and Path layer internally)

I did a random check on the scatterplot layer (before and after changes) and it seems to work well.

The only one I don't like is the solid polygon layer. It does this conversion twice - one for triangulation (earcut) and the other for rendering. I was thinking to move the geometry to the state object but it'd cause considerable changes to the code. Let me know if you have suggestions on how we could avoid this. Cheers

@gmoney1729 gmoney1729 changed the title Support separated (struct) coordinates for solid polygon layer Support separated (struct) coordinates for all applicable layers Feb 8, 2025
@@ -249,7 +251,10 @@ export class GeoArrowSolidPolygonLayer<
recordBatchIdx < geometryColumn.data.length;
recordBatchIdx++
) {
const polygonData = geometryColumn.data[recordBatchIdx];
let polygonData = geometryColumn.data[recordBatchIdx];
if (isGeomSeparate(polygonData)) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe let's just add a comment here, saying

TODO: Note here that we do this conversion twice - one for triangulation (earcut) and the other for rendering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@kylebarron kylebarron merged commit 9d60501 into geoarrow:main Feb 11, 2025
1 check passed
@kylebarron
Copy link
Member

Thank you!

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.

TypeError when trying to render geoparquet with multi-polygons
2 participants