Define == for Some by forwarding to the wrapped value

[Seelengrab]

I think this just makes sense - the equality of two Some depends on the equality of the wrapped value, once the Some is unwrapped. This ought to make it a bit nicer to work with Some.

[Seelengrab]

The tests failing on hashing/objectid of Some is a bit weird; nothing in this PR should be touching those, right?

[nalimilan]

This behavior is reasonable and I wish we had implemented it when adding Some. But now it changes a behavior that people may rely on, so adding the "minor change" label.

[Seelengrab]

You mean people rely on Some([1]) != Some([1]), due to this currently using the objectid fallback? I'd happily argue that relying on that is a bug.

[simeonschaub]

I'm not a fan of special-casing missing here, to me the missing case seems more like a symptom of the underlying issue that just calling == on the unwrapped values might not be the right behavior semantically. Maybe ::Some == ::Some forwarding to isequal on the unwrapped value is the more sound thing to do?

[Seelengrab]

Why would it not be the right behavior semantically? What behavior do you envision when passing a Some(missing)?

Using isequal in a == method (which has a different meaning) leads to issues when you have e.g. Some(Some(missing)) == Some(missing). Handling missing explicitly means that this will already return false on the first "layer" of Some, instead of ending up returning missing. Keep in mind that == is only supposed to return missing if one of its arguments is === missing, not an object wrapping missing. Replace Some with any wrapper like Ref and you get the same behavior - the fact that this is a wrapped object is meaningful here; the Someness is important.

[simeonschaub]

I'm not disagreeing with you on that. Where I'm coming from is just that users might have implemented their own types propagating similarly to Missing and it would be weird to require them to implement their own == methods on Some{MyType} which is arguably even type piracy

[Seelengrab]

That's a question for the contract of == (see #52484), not this PR though. For Some, there really can't be another meaning; if MyType propagates missing, so be it, the contract still holds.

[vtjnash]

We should probably merge #44678 instead since it is a bit more general than this. I am leaning towards doing that now, given the multiple attempts now at it

[Seelengrab]

That will make Some(missing) == Some(missing) return missing though, which is exactly what we don't want. Neither argument to == is a Missing, so it should not return missing, but true.

This PR really isn't trying to "solve" an invalidation problem for all types in existence at all, but a semantic problem for Some, which has a clear interpretation. I don't think this can be solved by adding yet more "unwrappers"; the situation is similar to the difficulties in defining a general "array unwrapper". You're not going to get around specialization here. Even with that PR, we'd still have to add the Missing special handling.

[nalimilan]

This debate is just a special case of the old discussion at #4648. As you can see no decision has been reached... Another discussion happened at #9381 (comment).

That will make Some(missing) == Some(missing) return missing though, which is exactly what we don't want. Neither argument to == is a Missing, so it should not return missing, but true.

As noted in a thread above, this currently doesn't apply to arrays and tuples:

julia> (missing,) == (missing,)
missing

So treating Some in a different way requires a justification and a docstring.

The current state of the PR is inconsistent as it doesn't prevent == from returning missing, in which case an error is thrown:

julia> Some([missing]) == Some([missing])
ERROR: MethodError: Cannot `convert` an object of type Missing to an object of type Bool

At least @simeonschaub's proposal of using isequal has the advantage of being simple to explain (no special casing of missing), potentially more useful (as you probably also don't want Some(NaN) == Some(NaN) to return false if you don't like == returning missing), and consistent with WeakRef as @vtjnash noted.

However none of the proposals would be completely consistent with the current docstring for == which says (language isn't super strict though):

  For collections, == is generally called recursively on
  all contents, though other properties (like the shape for arrays) may also
  be taken into account.

[Seelengrab]

As you wrote in the issue I opened:

The idea is that missing is returned if it's not possible to determine whether the two collections are equal or different, because by replacing missing values by some values they could be made either equal or different.

Some is not a collection. Some is a semantic wrapper to indicate that a value is not nothing. Two Some are == if their wrapped objects are ==. If the result of that is that Some(NaN) != Some(Nan), so be it. The only special casing necessary here is missing, because the semantics of the wrapper ("is there something distinct from nothing here?") trumps that of the wrapped value.

However none of the proposals would be completely consistent with the current docstring for == which says (language isn't super strict though):

Note the generally there. That part of the docstring doesn't give any guarantee or impose any requirement on an implementation whatsoever. The only unambiguous requirement is that missing is returned if either argument is exactly missing.

[Seelengrab]

Test failure is a random timeout - seems unrelated to this PR. Is there anything else needed here to get this merged?

[KristofferC]

Is there anything else needed here to get this merged?

There seems to be a lot of active discussion here so it seems a bit odd to merge it at this stage, no?

[adienes]

tbh missing semantics are already super inconsistent so whatever is decided here probably can't hurt either way

[nalimilan]

IMO the main concern isn't the inconsistency of missing, but of ==. Julia already has three concepts of equality (==, isequal and ===), and the definition of == one already varies quite a bit across collection (vectors, dicts, sets...) and wrapper types (Ref, WeakRef...). Adding a custom handling of missing would introduce a third definition which would be somewhat intermediate between == (which propagates missing and treats NaN as not equal to itself) and isequal (which does the reverse), and which would also be different from other wrapper types Ref and WeakRef. At least, if ==(::Some, ::Some) called isequal on the wrapped value as @simeonschaub proposed, it would be consistent with what happens for WeakRef (and AbstractSet FWIW).

By the way, we haven't discussed any concrete use case, which doesn't really help deciding which behavior would be the most useful in practice.

[Seelengrab]

By the way, we haven't discussed any concrete use case, which doesn't really help deciding which behavior would be the most useful in practice.

My concrete use case is that I'd like Some([0x1]) == Some([1]) to return true, and similar for other. It would be extremely confusing to have isequal semantics on something that had == called on itself. In other words, I require == semantics, and not hash-equality semantics (it's not helped of course that Julia insists on conflating these concepts at times).

So without calling == on its wrapped value, this PR is useless to me.

[Seelengrab]

Adding a custom handling of missing would introduce a third definition which would be somewhat intermediate between == (which propagates missing and treats NaN as not equal to itself) and isequal (which does the reverse), and which would also be different from other wrapper types Ref and WeakRef

This PR does not introduce new/unseen semantics:

julia> Set([NaN]) == Set([NaN])
true

julia> Set([missing]) == Set([missing])
true

I can only repeat myself: Other than "propagate missing if either argument is exactly missing is the defining property of ==, the rest of the equality is dependent entirely on the type. As such, I really don't understand the sudden kerfuffle with wanting to make == consistent across entirely nebulous concepts such as "collection".

[nalimilan]

Yet on this PR:

julia> Some(NaN) == Some(NaN)
false

julia> Some([NaN]) == Some([NaN])
false

[Seelengrab]

Yes, because for the purposes of == these are not equal when forwarding ==. If someone doesn't want the floating point behavior, they can use isequal.

Now, you might say "why doesn't that apply to missing?" and my response would be "Because you shouldn't have gotten Some(missing) in the first place". missing is infectious; it should be propagated by everything under the sun, right? So how did you manage to wrap it in the first place? I can't think of an example where Some(missing) comes up organically, but I can think of plenty of examples where Some(NaN) could numerically.

[Seelengrab]

Alright, since I care about the behavior of the non-missing case more than the cases involving missing, I've removed the specialization so that missing is propagated on Some(missing). @nalimilan, is this better from your POV now?

[Seelengrab]

It's been a month and I've been told that bumping & pinging people is the socially accepted way. So, @nalimilan, is this PR ready to go with the (now implemented) missing behavior you wanted, or does this need something else?

[adienes]

what about instead defining a new isequal to forward isequal on the value, and leaving == alone? @Seelengrab that will give you the desired isequal(Some([0x1]), Some([1])) # true. But as-is, I don't think this PR should merge

seems like all other options in this thread have problems:

• ==(a::Some, b::Some) = a.value == b.value
  • I think returning missing for Some(missing) is super strange, as it does not match the precedent of Ref or Val. nor does it match the proposed contract for == in Generalize == Documentation to not be Statistics-Specific #53024 . also what if some weird type is returned like ::Variable from a symbolics package?
• ==(a::Some, b::Some) = isequal(a.value, b.value)
  • this is definitely not great, because if the user wants ==, they could be pretty surprised that isequal is called? especially given that isequal explicitly states in its docstring that it forwards to ==, so making this the other way around is odd
• forward ==, but special case Missing to be false (original PR)
  • somewhat reasonable, but still "breaks" on symbolics packages?

[Seelengrab]

I don't know what these failures are about - they seem unrelated to these changes. It's been almost another month, so here's another bump.

[Seelengrab]

Failures are from Pkg, seem unrelated? Also, bump again for lack of activity/review.

[Seelengrab]

Bumpo Nr. 5 🕺

[LilithHafner]

@nanosoldier runtests() for the minor change

[LilithHafner]

Triage thinks this is reasonable (but that defining methods like this for +, *, etc. would not be good, equality is special in that it is a really basic operation)

[nanosoldier]

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

[LilithHafner]

Nano-soldier looks clean at a glance

[Seelengrab]

Thank you!