Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

scalar mul/div for point #507

Closed
wants to merge 1 commit into from
Closed

Conversation

michaelkirk
Copy link
Member

fixes #171

@rmanoka
Copy link
Contributor

rmanoka commented Sep 7, 2020

@michaelkirk I'm not against this, but I think it is best to provide "vector space" operations (add, sub, mul/div by scalar) on Coordinate itself and not on Point. This is more of a preference, because it usually doesn't mean much to add points. There is also some relevant discussion in #15 (esp. #15 (comment)).

Even if you want to provide these on Point, we should also provide these operations on Coordinate as almost all composite geometries (LineString, etc) store Coordinate and not Point (I'm already doing this in #505). Providing the same on both seems repetitive, and may possibly prevent us from some future functionality on Point (eg. coordinate system attached to it, so it no longer makes sense to just add them only by coords).

@michaelkirk
Copy link
Member Author

I'm not against this, but I think it is best to provide "vector space" operations (add, sub, mul/div by scalar) on Coordinate itself and not on Point.

Good call. I'd support adding: add, sub, mul, div, neg to Coordinate.

may possibly prevent us from some future functionality on Point (eg. coordinate system attached to it, so it no longer makes sense to just add them only by coords).

That's a reasonable concern, but unfortunately that ship has already sailed. add, sub, neg exist for Point for a while now.

Since add, sub, neg for Point already exist, div mul seemed like an obvious complement.

I want to avoid a situation where Point and Coord have separate subsets of vectors methods available to them, so I think either:

  1. we have redundant operations defined on both of them (which is my slight preference, breaking things the least for current users)

-or-

  1. deprecate the existing ones on Point, and eventually only support them on Coordinate

@michaelkirk
Copy link
Member Author

we have redundant operations defined on both of them (which is my slight preference, breaking things the least for current users)

Actually, I don't know that I prefer this method more. I could be talked out of it. It's just lame to break API's all the time. 😛

@michaelkirk
Copy link
Member Author

Looking again at #505, I'm happy to use what you have @rmanoka and just close this.

@michaelkirk michaelkirk closed this Sep 7, 2020
@rmanoka
Copy link
Contributor

rmanoka commented Sep 7, 2020

1. we have redundant operations defined on both of them (which is my slight preference, breaking things the least for current users)

This sounds fine to me. We can deprecate the ops for Point at a later point (ha!) if it makes sense.

Regarding #505, I missed out Div by scalar, so I'll copy your changes into that PR.

@michaelkirk michaelkirk deleted the mkirk/scalar-point-math branch September 8, 2020 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Primitive operations on Points
2 participants