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

BigUint should have count_ones() and similar methods #174

Closed
BartMassey opened this issue Nov 8, 2020 · 9 comments
Closed

BigUint should have count_ones() and similar methods #174

BartMassey opened this issue Nov 8, 2020 · 9 comments

Comments

@BartMassey
Copy link

I needed BigUint::count_ones() for a project, so I implemented it in terms of to_u32_digits().

fn count_ones(b: BigUint) -> u32 {
    b
        .to_u32_digits()
        .into_iter()
	.map(|d| d.count_ones())
        .sum()
}

But now I'm relying on the optimizer to clean up my code, plus the implementation should probably return u64 because it's pretty easily possible to have more than 4 billion ones in a BigUint so it's buggy. :-)

I'd be happy to put together some kind of pull request if there's some interest in one.

@cuviper
Copy link
Member

cuviper commented Nov 9, 2020

The iterators in #158 will at least let you do that without creating a temporary Vec, but there will still be some translation from the internal digit size, which is extraneous for your task. Internally iterating self.data would be ideal, so yes, I'm open to a pull request for BigUint::count_ones(&self) -> u64.

A harder question is whether we should do anything for BigInt, since negative numbers conceptually have an infinite prefix of 1s in twos-complement (though we store sign+magnitude). We could count ones in the magnitude instead, but I think it would be unfortunate to have such different semantics from iN::count_ones. I suppose folks can use bigint.magnitude().count_ones() if that's what they want.

RE: "and similar methods": For count_zeros, I guess we could define that as only counting less than the MSB 1. For leading_zeros we'd have the same problem of the infinite prefix, but BigUint::leading_ones should be okay. We do have trailing_zeros already, and fortunately that is even consistent between twos-complement and the magnitude. I think that would be mostly true for trailing_ones too, but BigInt == -1 would need to be an exception.

@tczajka
Copy link
Contributor

tczajka commented Nov 9, 2020

It's not true for trailing_ones, e.g. (-3).trailing_ones() == 1 while 3.trailing_ones() == 2.

@cuviper
Copy link
Member

cuviper commented Nov 9, 2020

@tczajka Ah, you're right, of course. We can still do BigUint::trailing_ones easily, but we'd have to take care with BigInt.

@BartMassey
Copy link
Author

I think leaving BigInt out for now is probably the path of least resistance. Given that BigInt is sign-magnitude the users can do what they want after the essentially-free conversion to BigUint.

Should count_ones() return u64 or usize? I could make the case either way: neither is ideal. We could make it return BigUint but that seems needlessly silly for most applications…

I'll try to get a PR together. Thanks much!

@BartMassey
Copy link
Author

I feel like leading_ones() is not all that useful the way I have implemented it. What is really wanted, I think, is the number of significant_bits() — that is, 1 + position of leftmost 1-bit in the BigUInt. That's what's needed for things like log2(). I can't think of any relevant leading_zeros() measure for BigUInt.

@BartMassey
Copy link
Author

BartMassey commented Nov 10, 2020

Any simple implementation of BigUint::trailing_ones() requires Rust 1.46 or later, but this library is required to compile with Rust 1.31. I stuck trailing_ones() behind an off-by-default feature; advice on how to proceed appreciated.

The obvious plan is to depend on dtolnay's rustversion crate, but I'm not adding that dependency without some discussion from the team.

@cuviper
Copy link
Member

cuviper commented Nov 10, 2020

I gave a comment on the PR about trailing_ones, which I think we can simply avoid, but generally I use my own autocfg crate in the build script, probing for features rather than hard-coding versions.

BartMassey added a commit to BartMassey-upstream/num-bigint that referenced this issue Nov 10, 2020
BartMassey added a commit to BartMassey-upstream/num-bigint that referenced this issue Nov 10, 2020
@BartMassey
Copy link
Author

Sounds good, thanks.

bors bot added a commit that referenced this issue Nov 14, 2020
175: added BigUint::count_ones(), trailing_ones() r=cuviper a=BartMassey

Added `BigUint::count_ones()`, `BigUint::trailing_ones()`. Also minimal tests for these and `BigUint::trailing_zeros()`.

Closes Issue #174 

Co-authored-by: Bart Massey <bart.massey@gmail.com>
@BartMassey
Copy link
Author

Thanks for merging #175 !

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

No branches or pull requests

3 participants