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

Inconsistent behavior around tolerances in isequal and isless #311

Open
hyrodium opened this issue Apr 13, 2024 · 5 comments
Open

Inconsistent behavior around tolerances in isequal and isless #311

hyrodium opened this issue Apr 13, 2024 · 5 comments

Comments

@hyrodium
Copy link
Contributor

Examples of inconsistent behaviors

I am trying to remove method isless(::Point, ::Point) as discussed in #300 (comment), and I found some inconsistent behavior in isequal and isless.

julia> using Luxor

julia> p1 = Point(1, 1)
Point(1.0, 1.0)

julia> p2 = Point(1, 2)
Point(1.0, 2.0)

julia> p3 = Point(0.99999999, 3)
Point(0.99999999, 3.0)

julia> p1 < p2 < p3 < p1, p1 < p1  # does not satisfy transitive relation
(true, false)
julia> p4 = Point(1.000000001, 3)
Point(1.000000001, 3.0)

julia> p5 = Point(1.000000008, 3)
Point(1.000000008, 3.0)

julia> p6 = Point(1.000000015, 3)
Point(1.000000015, 3.0)

julia> p4 == p5 == p6, p4 == p6  # does not satisfy transitive relation
(true, false)

Proposal on removing the tolerances

I think removing these tolerances would be better for the following points.

Specialized unique method can be removed.

This is related to the following TODO comment.

Luxor.jl/src/point.jl

Lines 160 to 172 in 6c271e9

# a unique that works better on points?
# I think this uses ==
# TODO perhaps unique(x -> round(x, sigdigits=13), myarray) ?
# "any implementation of unique with a tolerance will have some odd behaviors" Steven GJohnson
function Base.unique(pts::Array{Point,1})
apts = Point[]
for pt in pts
if pt apts
push!(apts, pt)
end
end
return apts
end

Base.hash will be consistent.

The Base.hash should be consistent with Base.isequal. Currently, isequal(x::Point, y::Point) does not imply hash(x)==hash(y).

julia> isequal(p4, p5)
true

julia> hash(p4) == hash(p5)
false

https://github.com/JuliaLang/julia/blob/396abe4fdc8615e162462910bfc16729be7c95ed/base/hashing.jl#L5-L9

@cormullion
Copy link
Member

For 4.0 or a future version?

@hyrodium
Copy link
Contributor Author

Removing the tolerances would be a breaking change, so I think this is for 4.0. (Or, remove the tolerances on v5 release?)

@cormullion
Copy link
Member

Cool. I want to release v4.0 on Monday (choosing a day at random 😂), so I’m happy to merge PRs if the tests pass. Otherwise issues can be addressed for the following release cycle.

@hyrodium
Copy link
Contributor Author

Thank you for notifying the release schedule!

@cormullion
Copy link
Member

Well it's not so much a schedule :) - but it's probably a good idea to make a release fairly soon.

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