Add get_x and get_y vectorized functions (Point-only) #37
Conversation
|
I see that R's |
+1 on that as that is also consistent with shapely. |
jorisvandenbossche
changed the title
There was a problem hiding this comment.
This is (almost) ready I guess?
Alternatively we could return NaN instead of returning an error for non-point geographies? Or expose a parameter that allows user choosing between the two behaviors (similarly to the SAFE prefix in bigquery)?
(also both functions can be added to the doc API reference).
src/coords.cpp
Outdated
| auto point = static_cast<Point*>(geog); | ||
| auto s2point = point->s2point(); | ||
| auto latlng = S2LatLng(std::move(s2point)); | ||
| double lat = latlng.lat().degrees(); | ||
| return lat; |
There was a problem hiding this comment.
We could also just call s2geography::s2_y.
There was a problem hiding this comment.
Yes, at the time of writing this PR I didn't realize that existed, in the meantime I had noticed that as well.
I think that also has the behaviour of returning NaN for non-point geometries, as you suggested, if we would use it as is, without any validation before calling it.
I am not entirely following what the implementation is doing though: https://github.com/paleolimbot/s2geography/blob/5b06341403ec2e030b2a254bfa96d8c3386296f3/src/s2geography/accessors.cc#L165-L178
When is it possible that you need that for loop in there? Or does that mean that if you have a collection of two points, it will just give you the x/y of the first point? (I would find that a bit unexpected behaviour)
There was a problem hiding this comment.
Yes I was a bit confused by the implementation too. Probably safer to keep raising an error in case of non-point geographies for now.
Co-authored-by: Benoit Bovy <benbovy@gmail.com>
Awaiting a more complex function to get coordinates in general, adding a simple get_lng/get_lat functions just for points (so those can easily be vectorized)