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

Conversation

@expenses
Copy link
Contributor

This would resolve #2926, but median_algorithm isn't currently used. I haven't written a test yet, but I'd like to if it's important enough.

@parity-cla-bot
Copy link

It looks like @expenses signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

1 similar comment
@parity-cla-bot
Copy link

It looks like @expenses signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@gavofyork gavofyork added the A0-please_review Pull request needs code review. label Sep 18, 2019
@expenses
Copy link
Contributor Author

expenses commented Sep 18, 2019

I ran a simple benchmark comparing getting the median with both select and unsafe_sort: https://gist.github.com/expenses/d2d84305aeaa512200676dc0198e9f81#file-bench-rs. Results:

test bench_select ... bench:       1,898 ns/iter (+/- 208)
test bench_sort   ... bench:       9,288 ns/iter (+/- 1,340)

@rphmeier
Copy link
Contributor

Thanks! Mind if I ice this until #3583 is in, since it contains a much larger change-set on BABE that this might conflict with?

@expenses
Copy link
Contributor Author

Of course!

@rphmeier rphmeier added A1-onice and removed A0-please_review Pull request needs code review. labels Sep 18, 2019
Copy link
Contributor

@Demi-Marie Demi-Marie left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

lgtm. will merge after #3583 (should be soon anyway).

Ashley and others added 2 commits September 21, 2019 10:51
Co-Authored-By: André Silva <[email protected]>
Co-Authored-By: André Silva <[email protected]>
@rphmeier rphmeier removed the A1-onice label Sep 23, 2019
@rphmeier
Copy link
Contributor

Thanks!

@rphmeier rphmeier merged commit 1f15800 into paritytech:master Sep 23, 2019
en pushed a commit to en/substrate that referenced this pull request Sep 24, 2019
* use pdqselect for median_algorithm selection instead of sorting the whole vec

* Make use of pqdselect clearer

Co-Authored-By: André Silva <[email protected]>

* Make use of pqdselect clearer

Co-Authored-By: André Silva <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use a selection algorithm instead of a sorting algorithm in median algorithm

6 participants