GeoInterface.jl icon indicating copy to clipboard operation
GeoInterface.jl copied to clipboard

skip `NaN` in extent calculation

Open rafaqz opened this issue 1 year ago • 6 comments

This PR fixes the problem where NaNs win over other coordinates to make NaN extents that don't work anywhere. Or do work in DimensionalData.jl and because NaN comparisons always return false, it just selects the maximum possible extent.

To do this I switch extrema to _nan_free_extrema, which turns out to be faster anyway.

This PR will be coupled with a PR for Extents.union and Extents.intersection to behave the same way, and for DimensionalData.jl to check for NaN extents for the rare few that still get through.

rafaqz avatar Sep 25 '24 22:09 rafaqz

The weird thing about this (that I just realised for Extents.jl as well) is that for (X=NaN, Y=1000.0) the 1000.0 will still effect the final Y extent. Maybe this is an extreme edge case and doesn't matter much, I'm not sure.

rafaqz avatar Sep 25 '24 22:09 rafaqz

@evetion would be good to get a review and merge on this

rafaqz avatar Nov 05 '24 02:11 rafaqz

LGTM, I haven't hit this specific use case yet. Can you document the behaviour in the docstring(test?) somewhere. All NaN points will still yield a NaN extent right?

evetion avatar Nov 22 '24 11:11 evetion

For sure, a doc comment would help. And yes all NaN still returns NaN

rafaqz avatar Nov 22 '24 12:11 rafaqz

We should ask what other ecosystems do at SDSL

asinghvi17 avatar Sep 15 '25 13:09 asinghvi17

I think having NaN anywhere is unwanted/unexpected, so either we can introduce a keyword to enable this behaviour, or warn when skipping NaNs. We might even error when all is NaN, as having a NaN Extent is unusable in the rest of the ecosystem?

evetion avatar Sep 15 '25 14:09 evetion