-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: Allow precision to be set in GeoArrowArrayWriter #101
Conversation
Looks good, thanks a lot! Maybe also update the default in |
BTW, I also changed the default of |
It's also the default in GEOS since 3.12 as far as I can tell. It's definitely more compact to use a flat multipoint but is there a specific use case for spherely for exporting the flat version? (I added the option in geoarrow I'm pretty sure so that I can test the reader or maybe because I had a bunch of test data in flat form). |
Oh nevermind, I see that I'm disagreeing with myself and I think I wrote that default when the GEOS default was the other way. I'll add the option. |
FWIW from spherely's point of view I don't really care about an option for it, as long as it would be consistent with recent GEOS (I don't think we would make it configurable for the user of spherely, we would just set it under the hood) |
src/geoarrow/geoarrow.h
Outdated
/// A precision value of -1 indicates that the largest precision that will roundtrip | ||
/// an ordinate value will be used. The default precision value is -1. | ||
/// The default precision value is 16. See GeoArrowWKTWriter for details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This -1 does not actually work?
(I remember that I was looking into it last week, because GEOS has something similar and ryu should have the option to trim automatically trailing 0's, but don't remember exactly the outcome of it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or the conclusion was that trimming is actually is by default, right? (GEOS has an option for it (and which recently was switched to be on by default), but here we just always trim trailing zeros. The precision then only determines how many decimals to show if they would not be trimmed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-1 probably works...I think it's passed to RYU (which is the same backend GEOS is using). GEOS also has a "fixed" mode I think which I didn't bother with here. We can also compile without RYU, in which case we use basically sprintf("%g")
or sprintf("%0.<precsion>f")
, but that shouldn't be used for anything except testing since it will give you locale-specific values (and it's slower). I think precision means something slightly different in both cases and I forget the details.
The precision then only determines how many decimals to show if they would not be trimmed)
Yes, I think so!
Needed to ensure users can pass this value through!
https://github.com/paleolimbot/s2geography/pull/30/files#r1817306002