search

JuliaApproximation / DomainSets.jl

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

VectorProductDomain -> ArrayProductDomain #118

Closed findmyway closed 1 year ago

daanhb commented 1 year ago

Would you care to comment on or motivate this change? Thanks

findmyway commented 1 year ago

Oh, sorry that I thought this was a natural extension to the VectorProductDomain. The type constraint of domains in a VectorProdctDomain seems to be a bit too strict to me.

daanhb commented 1 year ago

Well, the code change may look harmless, but I'm not sure that a cartesian product with an array structure - rather than a simple list/tuple/vector - is even well-defined. And I'd be surprised if it doesn't break assumptions in the code elsewhere, even if the tests pass.

Is this useful to you elsewhere? What are you trying to achieve, perhaps there is another way to do so. An array-like product of domains may simply be a domain of arrays, for example.

findmyway commented 1 year ago

And I'd be surprised if it doesn't break assumptions in the code elsewhere, even if the tests pass.

In theory, this shouldn't break existing code. (If we add another alias of const VectorProductDomain = ArrayProductDomain and a deprecation warning)

https://github.com/JuliaApproximation/DomainSets.jl/pull/118/files#diff-d69205cdc4426c920cf621f57954f39e33e3c490a95733e83b043ca108343994L148-L150

The main change here is relaxing the type constraint restriction of domains.

Is this useful to you elsewhere? What are you trying to achieve, perhaps there is another way to do so.

Yes, similar to https://github.com/JuliaApproximation/DomainSets.jl/issues/119 I may need the size info of the components in an ArrayProductDomain. Sure I can create a customized domain for my specific usage. Just think this change seems to be a natural extension for the existing definition of VectorProductDomain.

daanhb commented 1 year ago

That const would not be correct, because the types are not the same. Perhaps a more involved const statement can be found though.

Still, that's not what I was thinking of. DomainSets itself is written with the assumption that product domains are a single list of domains. Does the existing functionality "just work" with arrays of domains - I'm really not sure about that.

findmyway commented 1 year ago

Okay, I get it. Maybe we can figure it out later when someone else has similar requirement.

daanhb commented 1 year ago

For future reference, the thing to do here is to check that all uses of map in product.jl are fine, that to_internalpoint and to_externalpoint work as expected, and that the conversions between the product domains are fine. This may be achieved most simply by restricting the functionality to cases where the domains are a vector.