throw error for incompatible types in 'in' method

[daanhb]

The in method tries to promote the types of the point x and of the element type of a domain. It currently returns false if this promotion fails. The proposal is to throw an error instead, because comparing incompatible types is often not intended, and the change would catch more bugs (it was a bug that triggered this pull request).

The current version of DomainSets has this behaviour:

julia> using DomainSets

julia> 1 ∈ (0..1)^2
false

No error is thrown even though 1 clearly is not a point in the 2D plane. After the pull request this changes to:

julia> 1 ∈ (0..1)^2
ERROR: InexactError: convert(StaticArrays.SArray{Tuple{2},Int64,1,2}, 1)

This is an intrusive change, changing the semantics of in. However, I find that without it I explicitly have to check for the types of the elements myself in various places.

One could argue that in should return false when x does not have the right type. That is what Set in Base seems to do. But we are not consistently doing that anyway. For example:

julia> (1,2) ∈ 0..1.0
ERROR: MethodError: no method matching isless(::Float64, ::Tuple{Int64,Int64})

There are other examples: whenever the promotion of the domain to Domain{promote_type(typeof(x),eltype(domain)} fails, an error is thrown rather than returning false. These errors are helpful, because you immediately see that you are doing something possibly unintended at best, and simply wrong at worst.

[dlfivefifty]

No we should return false as in Base:

julia> "hi" in [1,2]
false

[daanhb]

I'd be perfectly fine with that inconsistency! :-)

It would be weird to make another choice in Base: the meaning of in is that there is an element y in the collection for which x==y. Base can't make any other assumptions about all possible iterable sets out there, there is not even a T in the definition of in. A validity check is pointless.

It's not so weird in Domain{T}, precisely because of the T. Mixing unrelated types is a strong hint of issues with your code or with the way a user invokes a method. In that case, I prefer an informative error over silent continuation with a wrong result, much like NaN's propagate. I also prefer letting simple code throw errors rather than making more complicated code to catch errors and then return false - that is what happens in IntervalSets and why it throws the error in my example.

The functionality does not have to be in in, and it also doesn't mean that in should generically be in charge of throwing errors either, though that would be simpler. I can make a strict_in or something like that, and use that instead. Its definition would be:

strict_in(x::T, domain::Domain{T}) where {T} = in(x, domain)

strict_in(x, domain) = strict_in(point_domain_promotion(x, domain)...)

point_domain_promotion(x::S, domain::Domain{T}) where {S,T} = convert(promote_type(T,S), x), convert(Domain{promote_type(S,T)}, domain)

[daanhb]

Btw, that strict_in is not in the pull request and I did not actually try it. It looks more elegant to me than what we have now, possibly even more efficient.

[daanhb]

Or there could be a simpler way:

in(x, domain::Domain) = in(promote_point_domain(x, domain)...)

promote_point_domain(x, domain::Domain) = promote_point_domain(x, domain, promote_type(typeof(x),eltype(domain))

promote_point_domain(x, domain::Domain, ::Type{T}) where {T} = convert(T, x), convert(Domain{T}, domain)

We can still intercept promote_point_domain(x, domain, ::Type{Any}) and do something with it (maybe return an empty domain?) so that in by default returns false.

In this case, we may no longer need indomain. The whole point of indomain was to be able to do some sanity checks at the level of Domain, so that subtypes do not need to worry about it. With the definitions above, each domain can decide which types of x it will accept. It should at the least implement in(x::T, mydomain::MyDomain{T}) where {T}, but it may choose to implement more or, like IntervalSets does, not worry about the types at all and just do something simple and efficient (that sometimes throws errors, which will be easy for the user to fix).

[daanhb]

Hmm, ditching indomain has unintended side-effects. For example, the line

in(x, ::UnitHyperSphere) = norm(x) == 1

would evaluate to true for any vector whose norm is 1. Even if the element type of the domain is real, this may include points on the complex unit circle. So, I decided against it.

[daanhb]

Pull request updated. The behaviour is as before, with false being returned whenever we detect incompatible types at the level of Domain:

julia> 1 ∈ (0..2.0)^2
false

However, point and domain promotion is now performed by the point_domain_promote method before in invokes indomain, and the promotion returns NoDomain if the eltype would be Any. Next, false is returned by this line:

indomain(x, domain::NoDomain) = false

Elsewhere, if I don't like this behaviour, I can redefine this to be:

julia> DomainSets.indomain(x, d::DomainSets.NoDomain) = throw(InexactError(:convert, eltype(d), x))

In summary, this pull request is now just a small cleanup of the code we already had.

[daanhb]

There was an issue with a special case for Euclidean domains that I had removed (because it seemed like an arbitrary special case): vectors were converted to SVector in this line. Hence the update. This is now fixed in a different and more general way, outlined below.

The code is now functionally equivalent to what we had, but more flexible. The rules are as follows:

• A Domain{T} is a collection of elements of type T. Membership in(x,domain) is true if there is a y in the domain such that x==y and false otherwise, regardless of the type of x and of T.
• This is the same behaviour as in in Base for any iterable collection. Domains can be seen as a continuous collection.
• By default, in(x,domain) promotes the type of x and the element type of the domain to a joined supertype and calls indomain as follows: in(x, domain::Domain) = indomain(point_domain_promote(x, domain)...)
• Concrete domains can implement indomain. An implementation should be provided at least for points of type T. They can control which types of points they want to support by specializing point_domain_promote, either by converting x or by letting it pass through unaltered. For example, by default Euclidean domains allow any abstract vector because that is so very convenient:

  point_domain_promote(x::AbstractVector, domain::EuclideanDomain) =
  convert(eltype(domain), x), domain

• Another example is that domains in the complex plane could opt to support vectors in R^2, simply by "promoting" them to a complex number.
• If there is no joined supertype (it is Any), point_domain_promote returns x, nothing instead of a point and domain and in subsequently returns false.

The conversions sometimes throw an error. There is a performance penalty associated with catching this error in order to return false, so we don't. It is also impossible to prevent concrete domains from implementing in and bypassing the promotion step. However, any case that gives problems in practice can be fixed by specializing point_domain_promote for the troublesome types and returning x, nothing.

I think this implementation completely avoids the use case for spaces and embeddings, so they can be removed now. The promotion is simpler and more flexible. Also, perhaps the difference between the "internal" and "external" element type of a product domain can now be removed, or at least simplified, by implementing the relevant promotions/conversions.

[daanhb]

I've disregarded the solution above in favour of handling a few well-defined cases, for which I will make a separate pull request that includes other changes. Silently returning false when the types are incompatible has continued to catch me by surprise over and over again. It makes developing and using DomainSets much harder. In the new code, I'll emit a warning.