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

Tuples of Vecs always compare as equal, even when they are not #115

Open
dzhang314 opened this issue Aug 5, 2023 · 10 comments
Open

Tuples of Vecs always compare as equal, even when they are not #115

dzhang314 opened this issue Aug 5, 2023 · 10 comments

Comments

@dzhang314
Copy link

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
Copy link
Collaborator

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

@KristofferC
Copy link
Collaborator

KristofferC commented Nov 7, 2023

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
Copy link

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

@KristofferC
Copy link
Collaborator

Probably yes.

@KristofferC
Copy link
Collaborator

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

@eschnett
Copy link
Owner

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
Copy link

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
Copy link
Owner

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
Copy link

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

@eschnett
Copy link
Owner

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

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

4 participants