JuliaGraphics / Luxor.jl

Simple drawings using vector graphics; Cairo "for tourists!"
http://juliagraphics.github.io/Luxor.jl/
Other
575 stars 72 forks source link

Inconsistent behavior around tolerances in `isequal` and `isless` #311

Open hyrodium opened 2 months ago

hyrodium commented 2 months ago

Examples of inconsistent behaviors

I am trying to remove method isless(::Point, ::Point) as discussed in https://github.com/JuliaGraphics/Luxor.jl/pull/300#issuecomment-2043250289, 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.

https://github.com/JuliaGraphics/Luxor.jl/blob/6c271e9f5cf5cf2f9502748cd08e4e8fd76c84af/src/point.jl#L160-L172

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 commented 2 months ago

For 4.0 or a future version?

hyrodium commented 2 months ago

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

cormullion commented 2 months ago

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 commented 2 months ago

Thank you for notifying the release schedule!

cormullion commented 2 months ago

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