Skip to content

Commit

Permalink
RFC: Immediately return true for equality checks if is(A,B)
Browse files Browse the repository at this point in the history 
Perhaps I'm missing something but why not immediately return true from these checks if the two objects are identical?  If this is valid there are likely other places this could happen too...but I thought I would first check.

Thanks,

Glen
  • Loading branch information
@GlenHertz
GlenHertz committed Oct 27, 2013
1 parent a4f429a commit f1cdc93
Showing 1 changed file with 2 additions and 0 deletions.
2 changes: 2 additions & 0 deletions base/abstractarray.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1031,6 +1031,7 @@ end
## Reductions and scans ##

function isequal(A::AbstractArray, B::AbstractArray)
if A === B return true end
if size(A) != size(B)
return false
end
Expand All @@ -1054,6 +1055,7 @@ function lexcmp(A::AbstractArray, B::AbstractArray)
end

function (==)(A::AbstractArray, B::AbstractArray)
if A === B return true end
if size(A) != size(B)
return false
end
Expand Down

7 comments on commit f1cdc93

@lindahua
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There do exist cases where is does not imply ==:

julia> a = [NaN]
1-element Array{Float64,1}:
 NaN

julia> is(a, a)
true

julia> a == a
false

@JeffBezanson
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's a good point. NaNs are the only case where == is stricter than any other comparison.

@lindahua
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IEEE 754 specifies that NaN is NOT equal to itself. Therefore any array that contains NaN is not equal to itself.

@lindahua
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For numeric correctness, I think we should revert this commit.

@GlenHertz
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it depends if Array == Array is checking that the arrays are equal or is it checking if all the elements are the elements inside the array are equal? This isn't an element-wise check so I would argue that it is checking if the arrays are equal (not element by element). For the other case you could use all(A .== B). If NaNs bubble up then any composite type that has a NaN would compare false against itself -- that sounds hard to stomach. This change doesn't change the meaning of how two NaNs are compared. Thoughts?

@StefanKarpinski
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing that seems justifiable is that A == B means the same thing as all(A .== B) but short circuits. I don't see that arrays containing NaNs being self-non-equal is any worse than NaNs being self-non-equal in the first place. As for composite types, I think those should really not be defined by default in general (see #4648). If you want to check array equality, you should use isequal instead.

@JeffBezanson
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that's OK since usually an array of numbers is itself considered a numeric object. Other containers might be a bit different, for instance:

julia> d={NaN=>2}
{NaN=>2}

julia> d==d
true

julia> d={1=>NaN}
{1=>NaN}

julia> d==d
false

I'd say this is correct, since isequal is all that matters for dict keys, but == would be expected to call itself for contained values.

Please sign in to comment.