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

ENH: support writing the geoarrow-based encodings of GeoParquet #3275

Merged

Conversation

jorisvandenbossche
Copy link
Member

@jorisvandenbossche jorisvandenbossche commented May 6, 2024

WIP, on top of #3219

Will probably break out the reading side separately as a first PR, since that does not rely on #3219 (reading side: #3278)

This implements the writing side for geoarrow encodings in GeoParquet (#3253)

@jorisvandenbossche jorisvandenbossche marked this pull request as ready for review May 24, 2024 16:53
@jorisvandenbossche jorisvandenbossche changed the title ENH: support the geoarrow-based encodings of GeoParquet ENH: support writing the geoarrow-based encodings of GeoParquet May 24, 2024
Copy link
Member

@martinfleis martinfleis left a comment

Choose a reason for hiding this comment

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

Looks fine, with my limited GeoArrow knowledge. Some parts of the code are reported as uncovered but at least the ValueError has an explicit test, so I am not sure how trustworthy it is here.

CHANGELOG.md Outdated Show resolved Hide resolved
@jorisvandenbossche
Copy link
Member Author

Some parts of the code are reported as uncovered but at least the ValueError has an explicit test, so I am not sure how trustworthy it is here.

Yeah, codecov is doing a bit strange the last days .. Most of the annotations I see are definitely covered

Co-authored-by: Martin Fleischmann <martin@martinfleischmann.net>
@kylebarron
Copy link
Contributor

How are you thinking of handling the fact that GeoParquet 1.1 hasn't been released yet? Are you thinking of holding this until 1.1 is released?

@jorisvandenbossche
Copy link
Member Author

I was planning to add an "experimental" note to the docstring (it's all opt-in), but yes ideally we would finalize the GeoParquet spec soon (same issue exists for GDAL ..)

@jorisvandenbossche
Copy link
Member Author

(the same issue is true for the bbox writing/reading, so was planning to do that as a follow-up)

We could also add a warning to make it more visible, but given it is purely opt-in a note in the docs might be sufficient?

@kylebarron
Copy link
Contributor

note in the docs might be sufficient

I think that's sufficient

@jorisvandenbossche
Copy link
Member Author

Going to merge this one giving there are a few other PRs that touch the same code

@jorisvandenbossche jorisvandenbossche merged commit fef0c24 into geopandas:main Jun 1, 2024
18 of 20 checks passed
@jorisvandenbossche jorisvandenbossche deleted the geoparquet-geoarrow branch June 1, 2024 10:14
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

3 participants