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

== vs isequal #18

Closed
colinxs opened this issue May 8, 2020 · 4 comments · Fixed by #45
Closed

== vs isequal #18

colinxs opened this issue May 8, 2020 · 4 comments · Fixed by #45

Comments

@colinxs
Copy link

colinxs commented May 8, 2020

I'm curious why AutoHashEquals defines == in terms of isequal. In the README it says:

we use isequal() because we want to match Inf values, etc.

but it seems like the following would make more sense:

Base.hash(a::Foo, h::UInt) = hash(a.b, hash(a.a, hash(:Foo, h)))
Base.(:(==))(a::Foo, b::Foo) = a.b == b.b && a.a == b.a && true
Base.isequal(a::Foo, b::Foo) = isequal(a.b, b.b) && isequal(a.a, b.a) && true

This should still guarantee that hash(a) == hash(b) implies isequal(a, b). Looking at the definitions in Base for AbstractArray it seems like this is correct: isequal(A, B) applies isequal element-wise and ==(A, B) applies == element-wise.

Code for reference:
Base.(:==)
Base.isequal

@goretkin
Copy link

I agree that the above definitions would make auto-hash-equal'd structs behave like other containers.

Compare (with current release of AutoHashEquals.jl)

julia> using AutoHashEquals

julia> @auto_hash_equals struct Foo{T}
       _::T
       end

julia> ==(Foo(0/0), Foo(0/0))
true

julia> isequal(Foo(0/0), Foo(0/0))
true

julia> ==(Foo(missing), Foo(missing))
true

julia> isequal(Foo(missing), Foo(missing))
true

to

julia> ==((0/0,),  (0/0,))
false

julia> isequal((0/0,),  (0/0,))
true

julia> ==((missing,),  (missing,))
missing

julia> isequal((missing,),  (missing,))
true

goretkin added a commit to goretkin/AutoHashEquals.jl that referenced this issue Jul 30, 2021
JuliaServices#18

Analogous to

```julia
julia> ==((missing,),  (missing,))
missing

julia> isequal((missing,),  (missing,))
true
```
@ericphanson
Copy link
Contributor

@gafter I saw you had a big overhaul of this package in #32; I was wondering, do you have an opinion on this issue?

@gafter
Copy link
Member

gafter commented Aug 19, 2023

My opinion is that this is likely a change that this package should and will make. Do you want to offer a PR?

@ericphanson
Copy link
Contributor

ericphanson commented Aug 19, 2023

Sure, I can do that. Should it be opt-in and non-breaking, or just a breaking change?

BTW: my reasoning for why this is the right choice is a tiny bit different than the original post, in that I don't think Array is the right comparison, but rather NamedTuple's. I think they are essentially the canonical "anonymous record type", using "record type" to refer to a struct that is defined directly by the data stored in its fields (in contrast to a struct used to implement another data structure, like the struct definition of a Dict), and that AutoHashEquals is most useful/relevant for these types. NamedTuples define == in terms of == of the entries, and the same with isequal. Also, Legolas.jl makes the same choices too, albeit with a bug: beacon-biosignals/Legolas.jl#101.

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

Successfully merging a pull request may close this issue.

4 participants