JuliaApproximation / DomainSets.jl

A Julia package for describing domains as continuous sets of elements
MIT License
72 stars 12 forks source link

ProductDomain should be more consistent #71

Closed dlfivefifty closed 3 years ago

dlfivefifty commented 3 years ago

I would expect these both to be SVector{2,Int}:

julia> DomainSets.ProductDomain(0..1, 2..3) |> eltype
StaticArrays.SArray{Tuple{2},Int64,1,2}

julia> DomainSets.ProductDomain(0..1, [2,3]) |> eltype # Why Tuple?
Tuple{Int64,Int64}
daanhb commented 3 years ago

The second case seems to trigger this fallback: https://github.com/JuliaApproximation/DomainSets.jl/blob/659b385f4020034ef3ca264e0f53cfd9dfc5e71a/src/generic/productdomain.jl#L48

Tuples are the default because it is the only safe choice. The reason the fallback is triggered is that [2,3] is not (yet) recognized as a Domain, it is only converted to a WrappedDomain later by the TupleProductDomain constructor. A solution may be to write instead:

ProductDomain(domains...) = _ProductDomain(map(Domain, domains)...)
_ProductDomain(domains...) = TupleProductDomain(domains...)
_ProductDomain(domains::VcatDomainElement...) = VcatDomain(domains...)

?

Perhaps conversion to a wrapped domain can also be avoided, but that is what currently happens anyway. An alternative may be to improve the logic of the ProductDomain constructor a bit and make it more robust.

dlfivefifty commented 3 years ago

I don't really need this yet so no rush, but perhaps when it comes up I can have a go and remove the need for WrappedDomain by making Domain and "interface".

daanhb commented 3 years ago

Meanwhile I checked and the "fix" above seems to work at least for the example you gave.

More generally, Domain is largely an interface already but not everywhere. In this case, for future reference, there are at least two issues here:

So for these two reasons it is currently not possible to avoid the wrapping of the array.