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 export default & modify default params #2

Merged
merged 5 commits into from Oct 6, 2017

Conversation

Projects
None yet
2 participants
@DenisCarriere
Contributor

DenisCarriere commented Oct 6, 2017

dependency for rbush (PR mourner/rbush#79)

Ref: mapbox/supercluster#62

**Note: Moved default params to the main quickselect function

@mourner

@DenisCarriere please see ba9702b with the discussion

@DenisCarriere

This comment has been minimized.

Show comment
Hide comment
@DenisCarriere

DenisCarriere Oct 6, 2017

Contributor

👍 My bad.. didn't follow that thread 😄 I'll submit a change

Contributor

DenisCarriere commented Oct 6, 2017

👍 My bad.. didn't follow that thread 😄 I'll submit a change

@DenisCarriere

This comment has been minimized.

Show comment
Hide comment
@DenisCarriere

DenisCarriere Oct 6, 2017

Contributor

@mourner Added main() to handle the default params, then quickselect executes recursively without calculating the default params.

Contributor

DenisCarriere commented Oct 6, 2017

@mourner Added main() to handle the default params, then quickselect executes recursively without calculating the default params.

@mourner

This comment has been minimized.

Show comment
Hide comment
@mourner

mourner Oct 6, 2017

Owner

Let's rename main to quickselectand quickselect to quickselectStep to keep stack traces nice.

Owner

mourner commented Oct 6, 2017

Let's rename main to quickselectand quickselect to quickselectStep to keep stack traces nice.

@DenisCarriere

This comment has been minimized.

Show comment
Hide comment
@DenisCarriere

DenisCarriere Oct 6, 2017

Contributor

👍 Make sense, didn't know how you wanted to rename that method, but I like it!

Contributor

DenisCarriere commented Oct 6, 2017

👍 Make sense, didn't know how you wanted to rename that method, but I like it!

@DenisCarriere

This comment has been minimized.

Show comment
Hide comment
@DenisCarriere

DenisCarriere Oct 6, 2017

Contributor

Done 🎉

Contributor

DenisCarriere commented Oct 6, 2017

Done 🎉

@mourner

This comment has been minimized.

Show comment
Hide comment
@mourner

mourner Oct 6, 2017

Owner

@DenisCarriere you forgot to rename the internal quickselect call. It doesn't trigger failures but it's a perf hit.

Owner

mourner commented Oct 6, 2017

@DenisCarriere you forgot to rename the internal quickselect call. It doesn't trigger failures but it's a perf hit.

@DenisCarriere

This comment has been minimized.

Show comment
Hide comment
@DenisCarriere

DenisCarriere Oct 6, 2017

Contributor

Ops! True 🤦‍♂️

Contributor

DenisCarriere commented Oct 6, 2017

Ops! True 🤦‍♂️

@DenisCarriere

This comment has been minimized.

Show comment
Hide comment
@DenisCarriere

DenisCarriere Oct 6, 2017

Contributor

Renamed internal quickselect

Contributor

DenisCarriere commented Oct 6, 2017

Renamed internal quickselect

@mourner

mourner approved these changes Oct 6, 2017

@mourner mourner merged commit 65ad6cc into mourner:master Oct 6, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment