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

Introduce minPoints option #25

Merged
merged 4 commits into from
Feb 4, 2021
Merged

Introduce minPoints option #25

merged 4 commits into from
Feb 4, 2021

Conversation

zmiao
Copy link
Contributor

@zmiao zmiao commented Jun 3, 2020

@zmiao zmiao force-pushed the zmiao-minpoints-option branch from 02c4dd8 to d1c3581 Compare June 3, 2020 20:31
@zmiao zmiao changed the title Intorduce minPoints option Introduce minPoints option Jun 3, 2020
Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

👍

@zmiao zmiao force-pushed the zmiao-minpoints-option branch from d1c3581 to 7ee4e4e Compare June 4, 2020 11:57
@zmiao zmiao marked this pull request as ready for review June 4, 2020 13:21
Copy link
Contributor

@alexshalamov alexshalamov left a comment

Choose a reason for hiding this comment

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

Looks good 👍

One suggestion, maybe for a follow-up / investigation work. It looks like we could avoid expensive unordered_map copies between clusters.

if (num_points == 1) {
clusters.emplace_back(weight, 1, p.id, clusterProperties);
} else {
auto clusterProperties = p.properties ? *p.properties : property_map{};
Copy link
Contributor

Choose a reason for hiding this comment

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

clusterProperties only mutated if num_points >= options_.minPoints and if we have reduce function. Could we share p.properties between clusters if no mutation is required?

return;
}
b.visited = true;
clusters.emplace_back(b.pos, 1, b.id,
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably, b.properties can be shared between clusters? Could we change std::unique_ptr<property_map> properties{ nullptr } to std::shared_ptr<const property_map> properties{ nullptr }; and then try sharing properties between clusters if property map is not changed?

@zmiao
Copy link
Contributor Author

zmiao commented Feb 4, 2021

@alexshalamov Thanks for your suggestion! After some initial experiments for property map sharing as you suggested in the comments, I think it worths further investigation, and more tests need to be conducted to make sure it won't break anything. So I created an issue #26 to track it.

@zmiao zmiao merged commit 5f5eae8 into master Feb 4, 2021
@zmiao zmiao deleted the zmiao-minpoints-option branch February 4, 2021 09:20
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.

3 participants