Product center

[philip-stuckey]

added center for ProductDomain to resolve issue #137

[daanhb]

Looks good, thanks. You could put a test somewhere in the test_product_domain function (which starts here) to trigger a test of the line that was added. A test for rectangles is probably fine and would ensure that the case you initially cared about would continue to work.

[philip-stuckey]

I want to change some things (I think hcat was the wrong cat to use).

[daanhb]

Hmm, it might be a bit more complicated after all. While vcat works, it will return a Vector for rectangles, even if the eltype of the domain is SVector.

There is a generic routine to turn points of the composing domains into a point of the product domain, namely toexternalpoint. Could you try this definition: DomainSets.toexternalpoint(d, map(center, components(d)))?

[philip-stuckey]

so, there is a problem with that definition for rectangles with integer endpoints:

center((0..1)×(0..1))
ERROR: InexactError: Int64(0.5)
Stacktrace:
 [1] Int64
   @ ./float.jl:900 [inlined]
 [2] convert
   @ ./number.jl:7 [inlined]
 [3] macro expansion
   @ ~/.julia/packages/StaticArraysCore/U2Z1K/src/StaticArraysCore.jl:81 [inlined]
 [4] convert_ntuple
   @ ~/.julia/packages/StaticArraysCore/U2Z1K/src/StaticArraysCore.jl:77 [inlined]
 [5] SArray
   @ ~/.julia/packages/StaticArraysCore/U2Z1K/src/StaticArraysCore.jl:113 [inlined]
 [6] StaticArray
   @ ~/.julia/packages/StaticArrays/O6dgq/src/convert.jl:167 [inlined]
 [7] toexternalpoint
   @ ~/Projects/DomainSets.jl/src/generic/lazy.jl:21 [inlined]
 [8] center(d::Rectangle{StaticArraysCore.SVector{2, Int64}})
   @ DomainSets ~/Projects/DomainSets.jl/src/generic/productdomain.jl:54
 [9] top-level scope
   @ REPL[5]:1

My intuition is that we want the vectors to be the same type even if the underlying scalar field is different so even though center((0..1)×(0..1)):SVector{2, Float64} even though eltype((0..1)×(0..1))==SVector{2, Int64}. I just don't know how to specify that generally.

[philip-stuckey]

so I'm just spitballing here, but a function like bar(d::Domain{T}, y::U) where {T, U} = promote_type(T, U)(y) seems to work. bar(d, map(center, components(d))) gives SVector{2, Float64}(0.5,0.5) when d = (0..1)×(0..1).

[daanhb]

I'm actually okay with the error thrown when using Integer intervals. Either the user intends to use integers, in which case the center is not defined, or the user wants to use floating point numbers, in which case the solution is not to use integer intervals in the first place and the error signals the problem. I.e., the solution here is not to fix center for products of integer intervals, but to prevent the latter from being constructed in the first place. (There is an ongoing discussion about changing that behaviour but there are valid arguments either way.)

[philip-stuckey]

ok, that's probably fine then. It id a bit annoying because I'll have to remember to that that specific InexactError is the result of a rectangle made of intervals with integer bounds. But probably that relates to the bigger issue that I don't really want to get into (probably the pedantic thing to do would be to open an issue about tointernalpoint or something. but I;m not feeling that pedantic today).

[codecov[bot]]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (92f72b0) 85.70% compared to head (766c780) 85.70%.

:exclamation: Current head 766c780 differs from pull request most recent head b12bca9. Consider uploading reports for the commit b12bca9 to get more accurate results

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #138 +/- ## ======================================= Coverage 85.70% 85.70% ======================================= Files 34 34 Lines 2658 2659 +1 ======================================= + Hits 2278 2279 +1 Misses 380 380 ``` | [Impacted Files](https://app.codecov.io/gh/JuliaApproximation/DomainSets.jl/pull/138?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaApproximation) | Coverage Δ | | |---|---|---| | [src/DomainSets.jl](https://app.codecov.io/gh/JuliaApproximation/DomainSets.jl/pull/138?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaApproximation#diff-c3JjL0RvbWFpblNldHMuamw=) | `100.00% <ø> (ø)` | | | [src/generic/productdomain.jl](https://app.codecov.io/gh/JuliaApproximation/DomainSets.jl/pull/138?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaApproximation#diff-c3JjL2dlbmVyaWMvcHJvZHVjdGRvbWFpbi5qbA==) | `94.17% <100.00%> (+0.05%)` | :arrow_up: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[daanhb]

I'd merge, but following my suggestion now the code has an unnecessary DomainSets. qualifier (right?) and used components again rather than factors. The ratio of commits is already high compared to the number of lines affected anyway, how about one more :-) :-)

[daanhb]

Perhaps make it version 0.6.7 in Project.toml while you're at it?