Skip to content

Add Average trait with integer average computation #20

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

Closed
wants to merge 4 commits into from
Closed

Add Average trait with integer average computation #20

wants to merge 4 commits into from

Conversation

althonos
Copy link
Contributor

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

althonos commented Apr 6, 2019

bors retry

@bors
Copy link
Contributor

bors bot commented Apr 6, 2019

🔒 Permission denied

Existing reviewers: click here to make althonos a reviewer

Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

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

Hi, sorry for the late review. This is a really neat trick!

Is there a reason you chose to make it a new trait, rather than adding to Integer? Especially since you provide a blanket implementation -- you can add the same on Integer, just with specific where clauses added to those new methods. That also makes it possible for someone to override with an even better version for their own type.

A few more tests would be nice to increase the floor/ceil coverage, like averaging a small negative with a big positive, and vice versa.

@althonos
Copy link
Contributor Author

Hi !

Is there a reason you chose to make it a new trait, rather than adding to Integer? Especially since you provide a blanket implementation -- you can add the same on Integer, just with specific where clauses added to those new methods. That also makes it possible for someone to override with an even better version for their own type.

The blanket implementation needs BitAnd<&'b I, Output = I> + BitOr<&'b I, Output = I> + BitXor<&'b I, Output = I>, but that may not be needed a specialized implementation, so I believed adding this to Integer directly would make these traits necessary all the time.

I'll fix the other remarks this week.

@althonos
Copy link
Contributor Author

Closing in favour of #31.

@althonos althonos closed this Jan 14, 2020
bors bot added a commit that referenced this pull request Mar 6, 2020
31: Add Average trait with integer average computation  r=cuviper a=althonos

*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.

Co-authored-by: Martin Larralde <[email protected]>
Co-authored-by: Josh Stone <[email protected]>
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