skip NaN in union and intersection

[rafaqz]

Sister PR to JuliaGeo/GeoInterface.jl#166

@asinghvi17

[rafaqz]

Hmm maybe this is wrong... if there is any NaN in an extent maybe the whole thing should be throw out? Currently this just throws out the individual NaN

[asinghvi17]

Hmm I'm not sure there. It's not even that throwing out the extent is the safer option, since it means the spatial dimension is constrained to the non-NaN extent...

[rafaqz]

Yeah I think youre right, we just keep non-nan and thats as good as it can be, there are problems either way

[rafaqz]

Also is this breaking? I feel like its a pretty grey area, nan bounds are just undefined behaviour, and we can change undefined behaviour

[asinghvi17]

I wouldn't call this breaking for exactly that reason

[rafaqz]

@evetion would be good to get your perspective on if this makes sense

[evetion]

I'm unsure about this, it's a grey area to be sure, and one might handle the spatial/non-spatial cases differently.

NaN spatially is akin to a missing coordinate. We have a skipmissing for that matter, so for calculating extents, skipping nan makes sense. However, if the nan ends up in an extent, it implies there was only a single coordinate (or none) in that dimensions. I'd say we error out, because there's no (or an unknown) extent in that dimension then.

Spatially, X and Y are intertwined, so an XY Extent that has a NaN is useless in the sense that most spatial operations (intersect, etc) can't work. If you're only interested in a single dimension, probably non-spatial, I guess one could handle it, but I would write down some use cases before implementing it, as most of the downstream behaviour is undefined.

Besides, in this PR the following snippet is unrealistic: Y=(1.0, NaN) no? How do you end up with a NaN for the max part and not also one for the min part? In that sense, replacing it by Y=(missing, missing) might almost be clearer.

TLDR: I don't think this makes sense, except for skipping nan numbers (akin to skipmissing) in calculating extents.

[rafaqz]

The thing is union is or can be used for calculating total extents so this is exactly that skipmissing case I'm thinking of! intersection is just for consistency.

missing was not an option as it's type unstable and that will propagate everywhere.

Currently NaN pass through and returns NaN for lower or upper whichever was NaN but not both. So erroring really would be a breaking change.

Are you suggesting union should just error on any NaN? Or it stays as-is?

To clarify this PR is making the minimum possible changes that reduce NaN propagation without much of a performance hit or breaking some behaviour

[asinghvi17]

bump @evetion

[evetion]

I think this should be closed. JuliaGeo/GeoInterface.jl#166 should prevent Extents with NaN occuring, so this would not be needed.

I think (geospatial) set operations on NaN coordinates are undefined.