Skip to content

added count_ones function #5

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

Merged
merged 3 commits into from
Oct 21, 2016
Merged

added count_ones function #5

merged 3 commits into from
Oct 21, 2016

Conversation

fuine
Copy link
Contributor

@fuine fuine commented Oct 12, 2016

This solves #4. Please note that proposed implementation can most probably be further optimized (for example if the remainder is 0 then we can completely skip mask creation and sum modification). Please let me know what do you think about this @bluss.

pub fn count_ones<T: IndexRange>(&self, range: T) -> usize
{
let start = range.start().unwrap_or(0);
let end = range.end().unwrap_or(self.length - 1);
Copy link
Member

Choose a reason for hiding this comment

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

It should be a half open range (start inclusive, end exclusive), this way it's consistent with how slicing a vec works for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please take a look at the test below, especially at these assertions:

assert_eq!(fb.count_ones(70..95), 1);
assert_eq!(fb.count_ones(70..96), 2);

I think it does what you expect it to do (ranges work just like in vec)

let mask_first_block: Block = mask(first_rem);
let mask_last_block: Block = mask(last_rem);
sum += (self.data[last_block] & mask_last_block).count_ones() as usize;
sum -= (self.data[first_block] & mask_first_block).count_ones() as usize;
Copy link
Member

Choose a reason for hiding this comment

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

Can both of these masks be correct at the same time? One needs to include the start of a block, the other the end of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks suspicious but extensive debug printing (praise {:032b}) shows that it is indeed correct. Both masks in principle do the same - they leave bits on the beginning of the block. Then these bits get counted. For the first block you need to subtract their count (because you already counted them in during the first loop iteration, and they are not needed as they don't lie in the desired range). For the last block you simply need to add the number of ones in the remaining part and so addition is used.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation

@fuine
Copy link
Contributor Author

fuine commented Oct 21, 2016

Sorry for the delay on the review, apparently i didn't submit my comments when i originally wrote them...

let start = range.start().unwrap_or(0);
let end = range.end().unwrap_or(self.length - 1);
// range makes sure that range.start <= range.end
assert!(end < self.length);
Copy link
Member

Choose a reason for hiding this comment

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

This is the only part that is not like slicing a vec

Copy link
Contributor Author

@fuine fuine Oct 21, 2016

Choose a reason for hiding this comment

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

So you propose that we get rid of that assertion, or maybe change error message to the one resembling usual slicing error with index outside of bounds? When you use index outside of bounds you get the message thread 'main' panicked at 'index out of bounds: the len is 0 but the index is 0', ../ src/libcollections/vec.rs:1306

Copy link
Member

Choose a reason for hiding this comment

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

Slicing a vector permits "end == self.len()", so this should too.

Copy link
Member

Choose a reason for hiding this comment

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

And start == end == self.len() too for that matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah i see it now. I'm not sure why i wrote it incorrectly, but it's fixed now. Thanks for heads up!

@bluss
Copy link
Member

bluss commented Oct 21, 2016

Great. It still needs the start <= end actually. Unfortunately nothing checks that for us.

diff --git a/src/lib.rs b/src/lib.rs
index 8cb7b45..8cd25dd 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -148,8 +148,7 @@ impl FixedBitSet
     {
         let start = range.start().unwrap_or(0);
         let end = range.end().unwrap_or(self.length);
-        // range makes sure that range.start <= range.end
-        assert!(end <= self.length);
+        assert!(start <= end && end <= self.length);
         let (first_block, first_rem) = div_rem(start, BITS);
         let (last_block, last_rem) = div_rem(end, BITS);
         let mut sum = 0usize;
@@ -300,6 +299,23 @@ fn count_ones() {
     assert_eq!(fb.count_ones(70..96), 2);
     assert_eq!(fb.count_ones(70..99), 2);
     assert_eq!(fb.count_ones(..), 9);
+    assert_eq!(fb.count_ones(0..100), 9);
+    assert_eq!(fb.count_ones(0..0), 0);
+    assert_eq!(fb.count_ones(100..100), 0);
+}
+
+#[should_panic]
+#[test]
+fn count_ones_oob() {
+    let fb = FixedBitSet::with_capacity(100);
+    fb.count_ones(90..101);
+}
+
+#[should_panic]
+#[test]
+fn count_ones_negative_range() {
+    let fb = FixedBitSet::with_capacity(100);
+    fb.count_ones(90..80);
 }

@fuine
Copy link
Contributor Author

fuine commented Oct 21, 2016

Fixed, thanks for the fix. I'm not sure why i assumed that start <= end, but it was indeed a mistake. Do you want me to squash commits before merge? Or do you still see some rough edges that should be polished?

@bluss
Copy link
Member

bluss commented Oct 21, 2016

It's fine now. Thanks for working on this.

@bluss bluss merged commit 0518747 into petgraph:master Oct 21, 2016
@fuine
Copy link
Contributor Author

fuine commented Oct 21, 2016

Thanks! As always working with you is a pleasure :)

@fuine fuine deleted the count_ones branch October 21, 2016 16:33
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