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

hashing is part unimplemented, part incorrect #56

Open
nsajko opened this issue Sep 23, 2024 · 5 comments
Open

hashing is part unimplemented, part incorrect #56

nsajko opened this issue Sep 23, 2024 · 5 comments

Comments

@nsajko
Copy link
Contributor

nsajko commented Sep 23, 2024

Test:

using Test, Infinities

@testset "hash versus floating-point infinity" begin
    @testset "one argument" begin
        @test hash(Inf) === hash(∞)
    end
    @testset "two arguments" begin
        for h  rand(UInt, 256)
            @test hash(Inf, h) === hash(∞, h)
        end
    end
end

Result:

Test Summary:                       | Fail  Error  Total   Time
hash versus floating-point infinity |    1    256    257  22.4s
  one argument                      |    1             1   1.6s
  two arguments                     |         256    256  20.9s
ERROR: Some tests did not pass: 0 passed, 1 failed, 256 errored, 0 broken.
@nsajko
Copy link
Contributor Author

nsajko commented Sep 23, 2024

The proper fix is to implement the two-argument hash method by simply delegating to hash(::Float32, ::UInt) (Float64 would work, too, but I guess hashing Float32 is faster than hashing Float64?).

NB: I plan on fixing this indirectly by:

  1. creating a package TypeDomainRealInfinities, implementing negative and positive infinity as singleton types correctly
  2. getting the new package under JuliaMath
  3. getting Infinities to depend on the package and redefining Infinity

@dlfivefifty
Copy link
Member

I don't see why creating new packages is needed for fixing hash.

Note Infinity represents a broader concept then a positive real infinity. For example if the real line or complex plane are compactified then Infinity would be fine to represent the infinity. But that said, originally Infinity was the same as InfiniteCardinality{0}, that is, it was a subtype of Integer. And so there was a lot of discussion that ended up with the current setup.

@nsajko
Copy link
Contributor Author

nsajko commented Sep 23, 2024

why creating new packages is needed for fixing hash

It's not. I just want a package with a clear and more narrow scope to depend on, so I thought I'd fix existing bugs while at it.

Note Infinity represents a broader concept then a positive real infinity.

But it's equal to floating-point Inf, so the hashes must be equal, too.

julia> using Infinities

julia> isequal(Inf, ∞)
true

julia> hash(Inf) == hash(∞)
false

@nsajko
Copy link
Contributor Author

nsajko commented Sep 23, 2024

Are the infinities not supposed to equal each other?

@dlfivefifty
Copy link
Member

Yes they are meant to be equal to each other. You'd have to go back to old discussion to understand why we wanted Infinity <: Number.... not sure I actually remember.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants