overload hash and equality #147
Conversation
src/geos_types.jl
Outdated
| function Base.:(==)(g1::AbstractGeometry, g2::AbstractGeometry) | ||
| (typeof(g1) == typeof(g2)) || return false | ||
| get_context(g1) == get_context(g2) || return false | ||
| g1.ptr == g2.ptr | ||
| end |
There was a problem hiding this comment.
Isn't this more like === than ==? This is false for two different instances of the same geometry, right? I see the GEOS C API has GEOSEquals_r and GEOSEqualsExact_r. Might they be more suitable for ==?
There was a problem hiding this comment.
sadly
pt1 = readgeom("POINT(0.12345 2.000 0.1)")
pt2 = readgeom("POINT(0.12345 2.000 0.10000001)")
are reported as equal from these libgeos methods even with zero tolerance.
Another possibility would be to check if coordinates are equal. What do you think @visr ?
There was a problem hiding this comment.
Is that equal because it falls under the precision? See also #66.
If libgeos says it's equal I'm not sure I want to argue that in a libgeos wrapper package ;)
There was a problem hiding this comment.
I refactored to use equals only to discover #152
There was a problem hiding this comment.
So considering #152 we probably should not use equals to define ==. Should we do something like
function Base.:(==)(g1::AbstractGeometry, g2::AbstractGeometry)
coordinates(g1) == coordinates(g2)
end
There was a problem hiding this comment.
Ignoring the z is pretty weird behaviour for == and I would guess not what most Julia users will expect from base object comparison?
Shouldn't we encourage the use of LibGEOS.equals to mean geos equality, and Base.== to mean more regular Julia equality:
Generic equality operator. Falls back to ===. Should be implemented for all types with a notion of equality, based on the abstract value that an instance represents. For example, all numeric types are compared by numeric value, ignoring type. Strings are compared as sequences of characters, ignoring encoding. For collections, == is generally called recursively on all contents, though other properties (like the shape for arrays) may also be taken into account.
I also think rotated triangles shouldn't be the same with ==. Thats LibGEOS.equals.
There was a problem hiding this comment.
It would be interesting to see what other wrappers do here. What are you proposing for ==? Comparing the coordinates as proposed above might be an option, especially if we can implement that in a somewhat fast way.
There was a problem hiding this comment.
I changed equality to coordinate comparison. The current implementation is slow. The reason is that it materializes every Point in the geometries, which means lots of heap allocations. If we are happy with the semantics of this but are unhappy with the performance I can reimplement it by directly working with pointers and thus eliding the heap allocs.
There was a problem hiding this comment.
Yeah I'm fine with the semantics. If you prefer to optimize performance in a separate PR that is also fine.
There was a problem hiding this comment.
Addressing performance in a follow-up PR sounds great.
|
LGTY @visr ? |
|
Would you be able to rebase it? |
|
Sorry no need, I just squeezed it. |
Before this PR we had:
etc.