eschnett / SIMD.jl

Explicit SIMD vector operations for Julia
Other
167 stars 35 forks source link

Tuples of `Vec`s always compare as equal, even when they are not #115

Open dzhang314 opened 1 year ago

dzhang314 commented 1 year ago

I ran into the following issue while working with NTuples of Vecs:

               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.9.2 (2023-07-05)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

julia> using SIMD

julia> (Vec(1, 2), Vec(3, 4)) == (Vec(5, 6), Vec(7, 8))
true

julia> (Vec(1, 2), Vec(3, 4)) != (Vec(5, 6), Vec(7, 8))
false

The tuples (Vec(1, 2), Vec(3, 4)) and (Vec(5, 6), Vec(7, 8)) are absolutely not the same, but when compared with ==, Julia thinks they are! This comes from an unfortunate quirk of the way Base._eq is defined in tuple.jl, where any comparison result that is not explicitly false is treated as true.

KristofferC commented 10 months ago

Oof, that is unfortunate. I'll see if I can tweak Base to fix this..

KristofferC commented 10 months ago

Or alternatively, we have to implement this method ourselves. Maybe a vectorized == was a mistake. You can't do stuff like

julia> [Vec(1, 2), Vec(3, 4)] == [Vec(5, 6), Vec(7, 8)]
ERROR: TypeError: non-boolean (Vec{2, Bool}) used in boolean context
MasonProtter commented 3 weeks ago

Maybe we should just make a ==ᵥ or a .== that does the vectorized version, and have regular == compare the whole Vec?

KristofferC commented 3 weeks ago

Probably yes.

KristofferC commented 3 weeks ago

But this should apply to eg Symbolics as well so maybe the Base behavior can be improved.

eschnett commented 2 weeks ago

We can overload tuple comparisons for SIMD vector elements. I don't think this would be type piracy because we own the SIMD types. This would be a simple approach and would do what people expect. We are explicitly circumventing an "unfortunate choice" in tuple comparisons.

MasonProtter commented 2 weeks ago

SIMD.jl does owns Vec, but it does not own Tuple{Vec}: https://docs.julialang.org/en/v1/manual/style-guide/#Don't-overload-methods-of-base-container-types

eschnett commented 2 weeks ago

Given the rationale there – "This would provide custom showing of vectors with a specific new element type. While tempting, this should be avoided. The trouble is that users will expect a well-known type like Vector() to behave in a certain way, and overly customizing its behavior can make it harder to work with." – one could argue that overloading tuple comparisons is fine here because we're correcting an unexpected behaviour coming from an unfortunate choice made in Base.

Of course, correcting this in Base would be better.

MasonProtter commented 2 weeks ago

Sure. Sometimes piracy is useful and needed (at least as a stopgap). I'm just pointing out that it is piracy.

eschnett commented 2 weeks ago

Yes, thank you. Indeed I did not realize that.