Skip to content
This repository has been archived by the owner on Nov 22, 2023. It is now read-only.

point arithmetic #136

Open
ajkeller34 opened this issue Aug 2, 2018 · 5 comments
Open

point arithmetic #136

ajkeller34 opened this issue Aug 2, 2018 · 5 comments

Comments

@ajkeller34
Copy link

There are a few surprises (at least to me) that can happen with broadcasted Point arithmetic:

julia> [Point(1,1), Point(2,2), Point(3,3)] .- Point(1,-1)
ERROR: DimensionMismatch("arrays could not be broadcast to a common size")

julia> [Point(1,1), Point(2,2)] .- Point(1,-1)
2-element Array{Point{2,Int64},1}:
 [0, 0]
 [3, 3]

I would have instead expected:

julia> [Point(1,1), Point(2,2), Point(3,3)] .- Point(1,-1)
3-element Array{Point{2,Int64},1}:
 [0, 2]
 [1, 3]
 [2, 4]

julia> [Point(1,1), Point(2,2)] .- Point(1,-1)
2-element Array{Point{2,Int64},1}:
 [0, 2]
 [1, 3]

One possible fix on julia 0.7 is to define something like:

for f in (:+, :-)
    @eval Broadcast.broadcasted(::typeof($f), a::AbstractArray, p::Point) =
        Broadcast.broadcasted($f, a, StaticArrays.Scalar{typeof(p)}((p,)))
    @eval Broadcast.broadcasted(::typeof($f), p::Point, a::AbstractArray) =
        Broadcast.broadcasted($f, StaticArrays.Scalar{typeof(p)}((p,)), a)
end

Though I haven't thought through the broader implications, this is how I'm getting around the problem in a package I have that defines its own Point types. I'd much rather use the Point type defined here so that I can have compatibility with your visualization packages.

@rdeits
Copy link
Contributor

rdeits commented Aug 13, 2018

Point is an abstract vector, so this is kind of expected. I'd say if you want your point to be treated as a scalar in a given broadcasting operation, just wrap it in a Ref or tuple:

julia> [Point(1,1), Point(2,2), Point(3,3)] .- (Point(1, -1),)
3-element Array{Point{2,Int64},1}:
 [0, 2]
 [1, 3]
 [2, 4]

That seems pretty consistent with the v0.7 way of forcing users to be somewhat explicit about what should be a "scalar" in a given broadcasting operation.

@ajkeller34
Copy link
Author

I see what you're saying, but what is more common: 1) given three points A, B, and P, wanting to add the x-coordinate of point P to both the x and y coordinates of point A, while also adding the y-coordinate of point P to the x and y coordinates of point B, or 2) wanting to add a point to a list of other points (i.e. translating a polygon)?

Does that first operation really make sense in the context of cartesian geometry? I guess I'm asking, would you ever want a point to broadcast as anything but a scalar? Perhaps I'm missing an obvious use case.

@rdeits
Copy link
Contributor

rdeits commented Aug 14, 2018

No, I think that's a reasonable question. I would venture that specifically overriding broadcasting for the case of Point + AbstractArray is going to be confusing for users, since it would result in Point + Set or Point + Generator returning something completely different than Point + AbstractArray. So perhaps the more central question is whether Point should always be scalar-like in broadcasting.

Perhaps you could try defining Base.broadcastable(p::Point) = Ref(p) ? That would make Point behave like a scalar in all broadcasting operations. Then we would have to think about what P .+ 1 would do. Currently it works (operating elementwise on P), but if Point broadcasts as a scalar then it will fail.

@ajkeller34
Copy link
Author

I very much agree. Personally, I would gladly have P .+ 1 fail if it meant we could broadcast Point as a scalar. It doesn't seem like much of a loss to give up a shorthand way of translating a point in one specific direction.

@aplavin
Copy link

aplavin commented Oct 27, 2018

I'm not sure if modifying this would help with other operations, like * and /? As of now I don't see a way of scaling an array of Points (or Vecs) by a Vec: [Point(1,1), Point(2,2), Point(3,3)] ./ (Vec(1, 1),) gives an array of 2x2 static matrices, and .* does not work at all.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants