Skip to content

Add bitset_count_and#206

Merged
giovannic merged 4 commits intodevfrom
feat/logi_size
Aug 12, 2025
Merged

Add bitset_count_and#206
giovannic merged 4 commits intodevfrom
feat/logi_size

Conversation

@cwhittaker1000
Copy link

A little while ago, @giovannic kindly helped me out with an issue I was facing and developed bitset_count_and(a, b) for a case in a model we've been developing. That function takes two bitsets and returns the size of the $and. This was motivated by a desire to avoid what we'd previously been doing, which looked like this:

x$copy()$and(y)$size()

The copy was necessary as we needed to not modify x. But it became very computationally expensive as we were having to do this lots of times (lots of copying). This then became:

bitset_count_and(x, y)

which saved us a lot of computational overhead.

I'm conscious individual has moved on several versions since this was done. Would it be possible to think about merging this into main? So we can benefit from the addition of bitset_count_and() and the other updates to individual? Thanks in advance!

@cwhittaker1000
Copy link
Author

Tagging @giovannic and @plietar for visibility! :)

@plietar
Copy link
Member

plietar commented Mar 14, 2025

This seems reasonable. Based on a quick search there's at least a handful of occurrences of this in malariasimulation. I'll try to spin up a PR there to see if there's any measurable impact.

Not sure I love the name though. The number of set bits is typically called size, not count, so I think we should stick to that. bitset_intersection_size seems more correct to me (but is a little more verbose).

What do you think?

IterableBitset& operator&=(const IterableBitset&);
IterableBitset& operator|=(const IterableBitset&);
IterableBitset& operator^=(const IterableBitset&);
size_t count_and(const IterableBitset&) const;
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't make a big difference, but this feels like it should be a static method in the class:

static size_t count_and(const IterableBitset&, const IterableBitset&);

@cwhittaker1000
Copy link
Author

Wanted to circle back on this @plietar and @giovannic - we’re beginning to prep the work that uses this branch for publication so would love to merge this if you think that’d be possible!

Naming it bitset_intersection_size makes sense to me Paul! Apologies for not replying before (wasn’t sure whether it was my place to weigh in on names as I’ve not been involved in package dev) - though will defer to you and Giovanni re naming - whatever you think is best is good by me!

Thanks in advance folks!

@giovannic giovannic changed the base branch from master to dev August 6, 2025 09:24
@cwhittaker1000
Copy link
Author

Gentle nudge on this - @giovannic, do you think getting this merged might be possible?

Copy link
Member

@giovannic giovannic left a comment

Choose a reason for hiding this comment

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

Good morning, I'll give this an approval and push this through since the tests appear to pass and the comments aren't severe.

I fixed up the documentation and and brought this up to date with dev to get the CI to pass.

Thanks and sorry for the delay on this!

@giovannic giovannic merged commit abfcdb4 into dev Aug 12, 2025
5 checks passed
@cwhittaker1000
Copy link
Author

Thanks so much @giovannic, hugely appreciated! :)

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.

3 participants