Skip to content

Add Average trait with integer average computation #31

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants