Skip to content

implement intersection and union#20

Merged
bluss merged 4 commits intopetgraph:masterfrom
jrraymond:feature/set-operations
Mar 26, 2018
Merged

implement intersection and union#20
bluss merged 4 commits intopetgraph:masterfrom
jrraymond:feature/set-operations

Conversation

@jrraymond
Copy link
Collaborator

No description provided.

@bluss
Copy link
Member

bluss commented Mar 21, 2018

Oh my thanks, this crate, that I've almost forgot about. Maybe you're interested in being co-maintainer?

This looks good. What do you think about this in relation to additional in-place union/intersection operations? (Those can be added later, as other method names?) Since HashSet has bit ops for these operations, we have precedent for that too. But in fixedbitset we keep things rather simple if we can..

@jrraymond
Copy link
Collaborator Author

jrraymond commented Mar 21, 2018

I would be interested in being a co-maintainer.

The HashSet container uses | and & to create construct new HashSets. The union and intersection functions return iterators over the elements. There are no functions which do a union or intersection in place (well, I guess extends is a union).

I think we should mirror hashset exactly. Consistent interfaces make development so much easier for users. I will update the PR rename the current union as | and intersection as &. I will also add union and intersection functions which return iterators. If we want in place unions and intersections later we can call them by another name.

@bluss
Copy link
Member

bluss commented Mar 21, 2018

I agree with following conventions yet I wouldn't want to change the existing methods in the FixedBitSet, they are sort of tailor made for the bit set and don't all match (Most visibly probably the insert method?)

@bluss
Copy link
Member

bluss commented Mar 21, 2018

I'd be happy to add you as co maintainer

@jrraymond
Copy link
Collaborator Author

Yes, I agree we shouldn't break backwards compatibility, but for the new methods in this PR (union&intersection) I think it is probably better to be consistent with HashSet?

@bluss
Copy link
Member

bluss commented Mar 21, 2018

Yes that sounds good. To critique HashSet, I don't agree with not having method names for their HashSet-returning union operation (but it makes naming easier if we don't need to invent new names for similar operations..)

@jrraymond
Copy link
Collaborator Author

Its really hard to come up with a name for | and & which doesn't clash with union or union_with :)

@bluss
Copy link
Member

bluss commented Mar 26, 2018

This reminds me, we should probably implement BitAndAssign and similar for the Rust set types (in std).

@bluss bluss merged commit 45853b3 into petgraph:master Mar 26, 2018
@bluss
Copy link
Member

bluss commented Mar 26, 2018

Thanks for this, I'll make a release

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