Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixup hash(::Real) broken by #49996 #50067

Merged
merged 4 commits into from
Jun 6, 2023
Merged

Fixup hash(::Real) broken by #49996 #50067

merged 4 commits into from
Jun 6, 2023

Conversation

LilithHafner
Copy link
Member

@LilithHafner LilithHafner commented Jun 5, 2023

I mishandled nonzero pow returned by decompose(::BigFloat).

Fixes #50065 and adds a test

@LilithHafner LilithHafner added bugfix This change fixes an existing bug hashing labels Jun 5, 2023
@vtjnash
Copy link
Member

vtjnash commented Jun 5, 2023

Please use meaningful titles for commits and PRs and not #'s

@oscardssmith oscardssmith changed the title Fixup for #49996 Fixup hash(::Real) broken by #49996 Jun 5, 2023
test/hashing.jl Outdated
@@ -310,3 +310,6 @@ struct AUnionParam{T<:Union{Nothing,Float32,Float64}} end
@test Type{AUnionParam{<:Union{Nothing,Float32,Float64}}} === Type{AUnionParam}
@test Type{AUnionParam.body}.hash == 0
@test Type{Base.Broadcast.Broadcasted}.hash != 0

# Issue #50065
@test hash(1.0) == hash(BigFloat(1.0))
Copy link
Member

Choose a reason for hiding this comment

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

add 1//1 and big(1) for good measure.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it makes sense to add BigFloat and BigInt to the massive cross-product at the top instead.

Copy link
Member

Choose a reason for hiding this comment

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

well I'm sure glad I suggested testing BigInt :)

@LilithHafner
Copy link
Member Author

My justification for changing the collisions bound and comment describing it is that the comment is misleading.

julia> collides = [];

julia> for T = types, S = types, x = vals
           a = coerce(T, x)
           b = coerce(S, x)
           eq = hash(a) == hash(b)
           #println("$(typeof(a)) $a")
           #println("$(typeof(b)) $b")
           if isequal(a, b)
               @test eq
           elseif eq
               push!(collides, (a,b)) 
           end
       end
       # each pair of types has one collision for these values

julia> using StatsBase; countmap(typeof.(collides))
Dict{DataType, Int64} with 40 entries:
  Tuple{UInt64, Int8}                      => 2
  Tuple{UInt64, Rational{Int16}}           => 2
  Tuple{Rational{Int8}, Rational{UInt64}}  => 2
  Tuple{Float32, Rational{UInt64}}         => 9
  Tuple{Float64, Rational{UInt64}}         => 14
                                          => 

On average, each pair of types has 0.947 collisions. Adding BigFloat and BigInt increases this average to 1.076. The new bound is exactly the number of current collisions.

@LilithHafner
Copy link
Member Author

LilithHafner commented Jun 5, 2023

New tests on this PR unearthed a hashing bug for BigInt: #50075. I'm going to skip BigInt tests for now.

@oscardssmith
Copy link
Member

Sounds good. #50076 should fix that part of this.

@LilithHafner
Copy link
Member Author

Test failure looks unrelated

@oscardssmith oscardssmith merged commit 53bcb39 into master Jun 6, 2023
@oscardssmith oscardssmith deleted the fix-50065 branch June 6, 2023 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug hashing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hash(BigFloat) broken after #49996
3 participants