Use IEEE comparison in binindex - #1010
Open
andreasnoack wants to merge 1 commit into
Open
Conversation
Pass `lt = <` to `searchsortedfirst`/`searchsortedlast` in `_edge_binindex` instead of relying on `isless`. Since `-0.0 < 0.0` is false, -0.0 and 0.0 are binned identically without normalizing the inputs, so `_normalize_zero`, the separate `AbstractRange` method, and the constructor check rejecting ranges containing -0.0 can all go. `<` is also cheaper than `isless`: `fit(Histogram)` is about 20% faster with range edges and about 2x faster with vector edges.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
binindexlocates the bin of an observation withsearchsortedfirst/searchsortedlast, which compare withislessby default. Underisless,-0.0sorts strictly below0.0, so an observation of-0.0would land in a different bin than0.0. The current code works around this by passingby=_normalize_zerofor vector edges, keeping a separateAbstractRangemethod withoutbyto avoid losing Base's arithmetic fast path, and rejecting ranges that contain-0.0in theHistogramconstructor since that method could not handle them.Passing
lt = <instead gives IEEE semantics directly:-0.0 < 0.0is false, so the two zeros are equal and no normalization is needed. NaN compares false with everything, so it still ends up outside the edges and is dropped bypush!as before. Base's arithmetic range method accepts the resultingLtordering, so the fast path is kept.Changes
_edge_binindexbecomes a single method usinglt = <;_normalize_zeroand theAbstractRangemethod are removed.-0.0is removed, since its reason no longer applies. Such ranges now bin identically to the corresponding ranges with0.0.@test_throwsfor-0.0ranges are replaced by equality tests, and explicit tests that NaN observations are dropped are added for bothclosedvalues and both edge types.<is also cheaper thanisless.fit(Histogram, v, edges)on 1e6 Float64 observations with 100 bins, on an M-series Mac:Vector{Float64}Downstream packages
Bin assignment is unchanged for all inputs except that ranges containing
-0.0are now accepted instead of throwing. The removed helper_normalize_zerowas internal. A GitHub code search found no package referencing_normalize_zeroor the removedArgumentErrormessage.Consumers of
StatsBase.Histogramthat were checked and go throughfit/binindexunchanged: Plots, StatsPlots, Makie (hist,stephist, datashader), AlgebraOfGraphics, UnicodePlots, PairPlots, BAT, FHist, RadiationSpectra, LegendSpecFits. None of them depends on the-0.0range check or on_normalize_zero.Before merging, any downstream package that would break because of this change must be fixed first. None has been identified so far; if one turns up it should be listed here.
🤖 Generated with Claude Code