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 Average trait with integer average computation #31

Merged
merged 8 commits into from
Mar 7, 2020
Merged

Add Average trait with integer average computation #31

merged 8 commits into from
Mar 7, 2020

Conversation

althonos
Copy link
Contributor

This is actually a copy of #20, I had to reopen a PR to address the fixes as I deleted the repository in between

Hi there !

This PR adds a new trait Average with two methods Average.average_ceil and Average.average_floor that uses bitwise operations to compute the average of two integers without overflowing. Reference is here, but this is probably known to people thanks to the Hacker's Delight.

Basically:

⌊(x+y)/2⌋ = (x&y) + ((x^y) >> 1)
⌈(x+y)/2⌉ = (x|y) - ((x^y) >> 1)

It comes with a blanket implementation for all Integer implementors that are closed under the -+|&^>> operators; in particular, this blanket implementation works for the BigInt and BigUint structs.

In terms of performance, this implementation is about 1.5 times faster than a naive implementation that checks for overflow, and as fast as an implementation that doesn't (i.e. (x+y)/2).

I'll add more tests for the std primitives, please do tell me if anything else needs to be changed.

@althonos
Copy link
Contributor Author

cc @cuviper, I had to reopen a PR, but in the meantime I fixed the review comments from the previous one.

@cuviper
Copy link
Member

cuviper commented Feb 1, 2020

Hi @althonos, this does look nice, thank you! More tests are always appreciated, if you're still planning to do that. The benchmarks might want to do a validation step of their expected results too, like the roots benches do -- the CI script runs test --benches on nightly for that purpose.

@althonos
Copy link
Contributor Author

althonos commented Feb 3, 2020

Hi @cuviper, I added a validation test for the benchmarks (at least for the methods that should be exact) plus proper unit tests, hopefully this is enough for now.

@cuviper
Copy link
Member

cuviper commented Mar 6, 2020

Looks great! I just added a few more boundary cases to the doc examples.

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 7, 2020

Build succeeded

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.

2 participants