Skip to content

Conversation

@rafaqz
Copy link
Member

@rafaqz rafaqz commented Sep 25, 2024

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 rafaqz changed the title skip nan in extent calculation skip NaN in extent calculation Sep 25, 2024
@rafaqz rafaqz requested a review from evetion September 25, 2024 22:01
@rafaqz rafaqz requested a review from asinghvi17 September 25, 2024 22:04
Co-authored-by: Anshul Singhvi <[email protected]>
@rafaqz
Copy link
Member Author

rafaqz commented Sep 25, 2024

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
Copy link
Member Author

rafaqz commented Nov 5, 2024

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

@evetion
Copy link
Member

evetion commented Nov 22, 2024

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?

@rafaqz
Copy link
Member Author

rafaqz commented Nov 22, 2024

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

Copy link
Member

@asinghvi17 asinghvi17 left a comment

Choose a reason for hiding this comment

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

LGTM, I added some documentation as well.

@asinghvi17 asinghvi17 moved this to In progress in SDSL 25 planning Sep 9, 2025
@asinghvi17 asinghvi17 moved this from In progress to In review in SDSL 25 planning Sep 9, 2025
@asinghvi17
Copy link
Member

We should ask what other ecosystems do at SDSL

@evetion
Copy link
Member

evetion commented Sep 15, 2025

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

3 participants