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

feat: Allow precision to be set in GeoArrowArrayWriter #101

Merged
merged 3 commits into from
Oct 28, 2024

Conversation

paleolimbot
Copy link
Contributor

Needed to ensure users can pass this value through!

https://github.com/paleolimbot/s2geography/pull/30/files#r1817306002

@jorisvandenbossche
Copy link
Contributor

Looks good, thanks a lot!

Maybe also update the default in GeoArrowWKTWriterInit to make it match?

@jorisvandenbossche
Copy link
Contributor

BTW, I also changed the default of use_flat_multipoint in paleolimbot/s2geography#30. Not sure if that it worth an official option with a setter, or we could also just update that (AFAIU the other way around is more "according to spec")

@paleolimbot
Copy link
Contributor Author

AFAIU the other way around is more "according to spec"

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).

@paleolimbot
Copy link
Contributor Author

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.

@paleolimbot paleolimbot merged commit d353e4d into geoarrow:main Oct 28, 2024
9 checks passed
@paleolimbot paleolimbot deleted the array-writer-options branch October 28, 2024 19:39
@jorisvandenbossche
Copy link
Contributor

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)

/// 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.
Copy link
Contributor

@jorisvandenbossche jorisvandenbossche Oct 28, 2024

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)

Copy link
Contributor

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)

Copy link
Contributor Author

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!

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.

2 participants