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

Move to arrow rs #3

Merged
merged 12 commits into from
Jan 30, 2024
Merged

Move to arrow rs #3

merged 12 commits into from
Jan 30, 2024

Conversation

nmandery
Copy link
Owner

arrow2 is currently unmaintained: jorgecarleitao/arrow2#1429

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
.map(|v| v.map(Geometry::from))
.collect::<Vec<_>>(),
))
let mut builder = WKBBuilder::with_capacity(WKBCapacity::new(self.len(), self.len()));
Copy link

Choose a reason for hiding this comment

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

btw, depending on the relative cost of repeated allocations to grow the wkb array vs the cost of materializing geometries from h3 objects, you may want to pre-calculate the exact capacities that you need for the WKBArray.

You can do that with WKBCapacity by calling add_polygon or from_polygons

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thant you for your review - that is a good point. I tried to address this with 4dcf0ca

impl_to_cells!(geoarrow::array::MultiLineStringArray<O>, O);
impl_to_cells!(geoarrow::array::MultiPointArray<O>, O);
impl_to_cells!(geoarrow::array::MultiPolygonArray<O>, O);
impl_to_cells!(geoarrow::array::PointArray);

Choose a reason for hiding this comment

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

Interesting! I've been wondering how to implement this! So far I've had separate hard-coded impls for PointArray https://github.com/geoarrow/geoarrow-rs/blob/3a2aaa883126274037cabaf46b1f5f6459938297/src/algorithm/geo/area.rs#L52-L63

I suppose this is slightly better, though you still have to duplicate the implementation in the macro.

Copy link
Owner Author

Choose a reason for hiding this comment

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

There still may be better ways to implement this. I just try to keep macro implementations not too complicated so I still understand them after coming back to them after a while ;)

}
})
self.get_as_geo(pos)
.map(|geom| geometry_to_cells(&geom, options))

Choose a reason for hiding this comment

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

It would be awesome in the future — if we're able to standardize geometry traits — if h3o is able to implement conversions via those traits. Then you wouldn't have to go through geo objects for all these conversions.

Copy link
Owner Author

Choose a reason for hiding this comment

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

There is a lot of potential in the geometry traits. In the case of H3 cells it may be a bit complicated as these can be interpreted as polygons and points.

@@ -33,9 +32,9 @@ where
opt.map(|array| {
array
.as_any()
.downcast_ref::<PrimitiveArray<u64>>()
.downcast_ref::<UInt64Array>()

Choose a reason for hiding this comment

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

btw you can use as_primitive if you want to save a few lines.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Nice - thanks

@nmandery nmandery marked this pull request as ready for review January 30, 2024 18:11
@nmandery nmandery merged commit 1491610 into main Jan 30, 2024
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

2 participants