search

yeesian / ArchGDAL.jl

A high level API for GDAL - Geospatial Data Abstraction Library
https://yeesian.github.io/ArchGDAL.jl/stable/
Other
141 stars 27 forks source link

fix inference on GI.getcoord #379

Closed rafaqz closed 1 year ago

rafaqz commented 1 year ago

Throw an error on wrong index to getcoord instead of returning nothing. This was pretty bad for inference and ran into bugs in base and other poorly inferenced functions in Meshes.jl to cause hangs.

@ErickChacon this should fix the other half of your inference bug. Thanks for persevering to find problems in both packages and Base!

visr commented 1 year ago

Looks fine to me. Could possibly be seen as a breaking change. But GeoInterface docs doesn't say much about what to return when a coordinate doesn't exists and this seems sensible. Only that for shorter tuples and vectors it throws an ArgumentError. I see GI.z and GI.m also return nothing for XY geometries, should that also be changed directly?

rafaqz commented 1 year ago

Yeah it should always error instead of nothing then the compiler has clean Float64

(Inference is giving Union{Float64,Nothing} currently)

yeesian commented 1 year ago

Thanks for this! LGTM modulo tests passing

rafaqz commented 1 year ago

Turns out half the problem was probably our horrific fallback for z in GeoInterface.jl, which could be hit because of a slightly wrong type used in the dispatch types in GI.z here. It was yet another source of nothing.

The fix means getcoord only accepts Integer now where before it would pass through Nothing. GI.z(geom) will also error if there is no z now, rather than returning nothing.

So maybe its a breaking change, IDK.