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

add centroid implementation for MultiPoint #322

Merged
merged 1 commit into from
Dec 21, 2018

Conversation

nberard
Copy link
Contributor

@nberard nberard commented Dec 17, 2018

Adds missing implementation of centroid on MultiPoint struct.

@datanel datanel requested review from frewsxcv and urschrei December 20, 2018 09:38
Copy link
Member

@urschrei urschrei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, apart from the nit

}
let sum = self.0.iter().fold(
Point::new(T::zero(), T::zero()),
|a: Point<T>, b: &Point<T>| Point::new(a.x() + b.x(), a.y() + b.y()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need b to be &Point<T>?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.iter() returns a Iterator<Item=&Point<T>> thus you can't really do any other thing (type inference might work here).

Copy link
Contributor

@TeXitoi TeXitoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe adding a test?

@nberard
Copy link
Contributor Author

nberard commented Dec 21, 2018

the test is in the comment ;)

@TeXitoi
Copy link
Contributor

TeXitoi commented Dec 21, 2018

That's OK for me, @urschrei I let you merge if everything is good on your side.

@urschrei
Copy link
Member

bors r+

bors bot added a commit that referenced this pull request Dec 21, 2018
322: add centroid implementation for MultiPoint r=urschrei a=nberard

Adds missing implementation of centroid on MultiPoint struct.

Co-authored-by: Nicolas Berard <nicolas.berard@canaltp.fr>
@bors
Copy link
Contributor

bors bot commented Dec 21, 2018

Build failed

frewsxcv referenced this pull request in georust/rstar Dec 21, 2018
@frewsxcv
Copy link
Member

Stoeoef/rstar@d4ca14e

@urschrei
Copy link
Member

bors retry

bors bot added a commit that referenced this pull request Dec 21, 2018
322: add centroid implementation for MultiPoint r=urschrei a=nberard

Adds missing implementation of centroid on MultiPoint struct.

Co-authored-by: Nicolas Berard <nicolas.berard@canaltp.fr>
@bors
Copy link
Contributor

bors bot commented Dec 21, 2018

Build succeeded

@bors bors bot merged commit 4a4819b into georust:master Dec 21, 2018
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.

4 participants