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

Add isless and make isequal consistent with hash #28

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andyferris
Copy link

@andyferris andyferris commented Jul 16, 2018

The current isequal won't work in a Dict because it violates the principle that isequal(a, b) implies hash(a) == hash(b). This is corrected to not special case empty intervals (unlike == where this makes perfect sense).

I also provide an isless function so that intervals can be sorted stabily and so-on.

@andyferris
Copy link
Author

The alternative of course would be to add a check for empty closed intervals in hash (and update isless accordingly). However, I think performance of Dict and sort might be slightly better with the approach here.

@timholy
Copy link
Member

timholy commented Sep 1, 2018

This came while I was traveling without internet, sorry it didn't get attention until now.

I understand your point about Dict performance, but I kind of think that the docstring implies that isequal is more "permissive," especially about missing values, than ==. Since an empty interval is kind of like a missing value, I worry this is counter to expectations. Consequently I would favor adding the check to hash.

@andyferris
Copy link
Author

OK. This seems consistent with how we treat empty ranges, arrays, etc.

@rofinn
Copy link

rofinn commented Apr 18, 2023

This is a pretty old PR, but do we still want to add the check to hash? I was also surprised by this as it was causing collisions.

julia> OpenInterval{Float64}(1, 3) == ClosedInterval{Float64}(1, 3)
false

julia> hash(OpenInterval{Float64}(1, 3)) == hash(ClosedInterval{Float64}(1, 3))
true

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 this pull request may close these issues.

3 participants