Updates 202606 - #90
Conversation
|
Hi @nmandery — I just took the opportunity of having Fable 5 back to dig a little into this PR and the migration work you did here. First of all: really nice job, this is a big update and it reads very cleanly — and CI is green across the board. While going through it I spotted a few small things that might be worth a look, roughly by severity: 1. cells = geometry_to_cells(shapely.Point(8.9, 47.2), 8)
cells_to_wkb_polygons(cells).type
# 0.22.0: arro3.core.DataType<LargeBinary>
# PR readthedocs/readthedocs.org#90: arro3.core.DataType<Binary>Besides the dtype change, WKB payloads > 2 GiB (~16M hexagon polygons) would panic on offset overflow, and it's inconsistent with 2. Empty geometries now yield nulls instead of empty cell lists. 3. The 4. Minor: the WKB output field name changed from If you don't mind, I can propose these improvements directly in the PR, and you can validate them later on. |
|
Hi @regislon Thanks for looking into this - these are all valid findings. Please just go ahead and start a PR to address these. For some of the issues like point 3 and the rasterh3 upgrade I also have started to prepare fixes, but so far that is not finished. |
keewis
left a comment
There was a problem hiding this comment.
some small nits, otherwise this looks good to me (and tests pass). Shall we merge this now and follow up with smaller PRs afterwards?
| use nom::IResult; | ||
| use nom::Parser; |
There was a problem hiding this comment.
| use nom::IResult; | |
| use nom::Parser; | |
| use nom::{IResult, Parser}; |
| ahash = "0.8" | ||
| arrow = { workspace = true } | ||
| geoarrow = { git = "https://github.com/geoarrow/geoarrow-rs", rev = "3ecf7dfc1816261b84f813eaf2a0174f2b5752d8", optional = true } | ||
| #geoarrow = { git = "https://github.com/geoarrow/geoarrow-rs", rev = "3ecf7dfc1816261b84f813eaf2a0174f2b5752d8", optional = true } |
There was a problem hiding this comment.
remove?
| #geoarrow = { git = "https://github.com/geoarrow/geoarrow-rs", rev = "3ecf7dfc1816261b84f813eaf2a0174f2b5752d8", optional = true } |
fixes readthedocs/readthedocs.org#86
Requires https://github.com/readthedocs/readthedocs-builder/issues/11