libcollections: Add a Multiset data structure. #15623

nham · 2014-07-11T23:31:07Z

This is not finished, but I want some feedback before I go too far on my own. This defines two new traits, Multiset and MutableMultiset. I currently have a partial implementation of these traits, TreeMultiset, based on TreeMap, and I plan to add another one based on HashMap. I have tried to make these traits and the TreeMultiset implementation closely correspond to the Set traits and TreeSet.

The TreeMultiset implementation is missing a few things, mostly some trait implementations. I also plan on adding a multiset sum operation, which is distinct from multiset union (see this Wikipedia page for reference).

One thing I want to add to the Multiset trait is a to_set method that will return the underlying set of the multiset, which would just be all the distinct values in the multiset with multiple occurrences removed. However, this currently doesn't seem possible, as noted in #8154.

Another thing I've wondered about: the Multiset trait could inherit the Set trait, but the way I've written it, MutableMultiset could not inherit MutableSet. One way this could be changed is to inherit MutableSet and use insert as a method to insert one occurrence (and remove as a method to remove one occurrence) and then add trait methods for inserting multiple occurrence, perhaps called insert_multiple/remove_multiple or insert_many/remove_many. I'm just not sure if this makes sense.

…eMap.

… take iterator parameter. This allows them to be used for both TreeSet and TreeMultiset.

Gankra · 2014-07-12T16:13:33Z

I don't think MutableMultiset should inherit MutableSet. Given a Set I expect the following to work:

set.insert(1); 
set.insert(1);
set.remove(&1);
assert!(!set.contains(&1));

And that's just not how a multiset works.

Gankra · 2014-07-12T17:20:39Z

src/libcollections/lib.rs

+
+    /// Add one occurrence of `value` to the multiset. Return true if the value
+    /// was not already present in the multiset.
+    fn insert_one(&mut self, value: T) -> bool {


Why not also have remove_one?

Inadvertent omission. I've just added it.

This is definitely a minor point, but it seems like insert_one/remove_one are probably the more common methods for someone to want. Would it be better to have insert/remove be insert_one/remove_one, and then have insert_many/remove_many? At very least, this would bring the multiset interface superficially closer to the set interface.

I think you're right. Most of the inserts/removes will probably be one at a time. I'll change this.

Add remove_one() method to MutableMultiset. Implement Show and Default for TreeMultiset. Add tests for count() and the Show implementation.

Gankra · 2014-07-13T16:36:13Z

src/libcollections/treemap.rs

+
+impl<T: Ord> Collection for TreeMultiset<T> {
+    #[inline]
+    fn len(&self) -> uint { self.map.len() }


Should len just return the number of keys, or the sum of the counts? I imagine it would be fairly easy and desirable to expose methods for both.

Thanks for catching this. I almost certainly copied it unthinkingly from the TreeSet implementation. I'm going to change len() to return the sum of the counts. Since the number of distinct values is probably a useful method to have, I'll add a method for that (though I'm currently not sure whether to add it to the trait or just to each implementation)

…evious version returned the number of distinct values.

Gankra · 2014-07-14T03:05:36Z

Should the possibility of splitting treemap.rs into multiple files be discussed here? It's getting a bit unwieldy, and could easily be separated into treemap, treeset, and treemultiset.

nham · 2014-07-15T20:55:49Z

With this patch, treemap.rs is currently at 2502 lines. By comparison, hashmap.rs is currently at 2520, so there is precedent for files this large. But perhaps it makes the most sense to put TreeSet and TreeMultiset into separate modules. I'm not sure here.

alexcrichton · 2014-07-16T15:57:08Z

This does seem to play nicely into the Set and MutableSet traits in that insert and remove operating on one element would be convenient. It may provide surprising results like @gankro pointed out, but I think that the symmetry would be more valuable.

For now, though, I'm not sure if adding new collections traits is the best option. The current traits are sort of ad-hoc and seem to be lacking an overall design in terms of interactions with one another and extensions into the future. Perhaps these methods could be inherent methods for now pending a redesign in the future? I don't think anyone is taking a generic Set or MutableSet today, so I don't think too much functionality will be lost.

…[insert|remove]_many

The collections crate is still in flux, so we are holding off on deciding the design of this trait.

nham · 2014-07-16T23:51:59Z

@alexcrichton Yeah, that makes sense. I've removed the traits.

nham · 2014-07-22T22:25:12Z

This should be ready for further review. TreeMultiset now implements all the traits that TreeSet implements and provides all the same methods as well, with the exception of move_iter (see below). I've also implemented the sum method for performing multiset sums.

For move_iter, we have a bit of an issue since the current design does not store multiple copies but simply a counter. In order to properly implement this I need to either change it to store multiple copies or only implement for cloneable values.

I should also point out that it's unclear to me how this could support both insert_many and a generic move_iter at the same time. If you're just giving a number for the number of occurrences an item should have, multiple values aren't available to be moved out by move_iter. So you either have to give up insert_many or make move_iter work only for Cloneable items.

Gankra · 2014-07-29T14:27:50Z

src/libcollections/treemap.rs

+        let mut mset = TreeMultiset::new();
+        mset.extend(iter);
+        mset
+    }


Any reason that from_iter isn't #[inline] but extend is?

The reason is that I copied and pasted function signatures from TreeSet and its from_iter method is not inlined! Same goes for much of the other methods.

Gankra · 2014-07-29T14:56:51Z

Part of me wonders if a "bulk" iterator that yields (item, count) would be desirable, but... so many iterators...

Gankra · 2014-07-29T14:59:37Z

src/libcollections/treemap.rs

+    }
+
+    /// Return true if the multiset has no elements in common with `other`.
+    pub fn is_disjoint(&self, other: &TreeMultiset<T>) -> bool {


While this implementation is elegant, would it not be radically more efficient to iterate the underlying TreeMaps, and compare counts? As an extreme case if I make two Multisets with 10000000 and 1000001 instances of x, this code will count all the way to 1000001 before terminating, where it could just do a single integer comparison.

Edit: I think it would be more readable, too.

Edit2: This comment was actually directed at is_subset, but there exists a similar very-inefficient input for this function.

pczarn · 2014-07-29T15:03:01Z

src/libcollections/treemap.rs

+        self.count -= 1;
+        self.current
+    }
+}


What's the difference between these implementations? Only the underlying iterators?

Yes, if I remember correctly that's the only difference. (same as for SetItems and RevSetItems)

Then you may write a single impl<'a, T, I: Iterator<(&'a T, &'a uint)>> Iterator<&'a T> for MultisetItems<'a, T, I> {, correct?

Yes, the only question is whether that makes the item too unwieldy for use. The same could be said for SetItems and RevSetItems (unifying them and parameterizing the iterator)

nham · 2014-08-09T02:00:26Z

There are some improvements that can be made to this, as @gankro and @pczarn have pointed out, but I'm no longer convinced that using TreeMap<T, uint> should be how TreeMultiset is implemented. There may still be a use for such a data structure, but perhaps it should be called a "CountedSet" or something. I'm currently working on a new implementation based on TreeMultimap, which will just be an AA tree that allows duplicate keys to be inserted. I'm closing this for now.

Gankra · 2014-08-09T02:06:25Z

@nham presumably you were dissatisfied with all colliding instances taking on the same concrete value?

nham · 2014-08-09T02:11:02Z

@gankro Yeah, only storing a count instead of multiple instances means that move_iter() cannot be implemented. I think TreeMultiset should store each instance individually, which I believe (but am not certain) is how C++'s std::multiset works.

Gankra · 2014-08-09T02:15:58Z

@nham: presumably TreeMultiMap<K,V> should just be a layer over TreeMap<K,Vec<V>>, then? And then TreeMultiSet<T> is just TreeMap<&T,Vec<T>>?

Edit: <>'s

nham · 2014-08-09T02:24:54Z

@gankro That is one way to go (TreeMultimap<K, V> = TreeMap<K, Vec<V>>, roughly), but I was thinking of using a slight variation on the current AA tree implementation where duplicate keys are allowed (so the left subtree contains values less than or equal to the current node, and when we try to insert a key that already exists, don't replace the old value, but actually insert a new node). I need to do some testing to see which version is better.

Gankra · 2014-08-09T02:31:13Z

@nham: avoiding more nodes seems like it'd be a big performance win in several ways (less frequent heap allocs). Although... the more I think about it the more I'm uncertain if my TreeMultiSet impl makes sense (can you get the ref out of the Vec correctly?). A duplicate key tree would be saner there, for sure.

Duplicate keys also has distinct semantics wrt popping nodes. If your keys have destructors, then TreeMap<K, Vec<V>> is very different from DuplicateTreeMap<K,V>. It would have to be a TreeMap<&K, Vec<(K,V)>> to have the same external-facing effects. Although if you assume keys are all worthless rubbish, then TreeMap<K, Vec<V>> will have serious space savings by only storing one key.

Not sure which is the desired behaviour!

nham · 2014-08-09T03:05:08Z

@gankro: Do I understand correctly that in your TreeMultimap, the first (key, value) gets inserted as (&key, vec!((key, value))). Then subsequent pairs (k, v) where the k equals key get pushed to the vector? Won't that prevent us from obtaining a mutable reference to the vector, since it is partially borrowed immutably?

Gankra · 2014-08-09T04:27:25Z

@nham: I believe you are correct. My implementation wouldn't work. You'd need a wrapper around a ptr, I guess.

fix: Don't skip closure captures after let-else As I understand that `return` was left there by accident. It caused capture analysis to skip the rest of the block after a let-else, and then missed captures caused incorrect results in borrowck, closure hints, layout calculation, etc. Fixes rust-lang#15623 I didn't understand why I using the example from rust-lang#15623 as-is doesn't work - I don't get the warnings unless I remove the `call_me()` call, even on the same commit as my own RA version which does show those warnings.

nham added 2 commits July 12, 2014 10:25

Add a Multiset trait and an implementation of that trait based on Tre…

c21a9b6

…eMap.

Change difference/symmetric-differenc/intersection/union iterators to…

0976d47

… take iterator parameter. This allows them to be used for both TreeSet and TreeMultiset.

Gankra reviewed Jul 12, 2014
View reviewed changes

Various additions.

2d22dcf

Add remove_one() method to MutableMultiset. Implement Show and Default for TreeMultiset. Add tests for count() and the Show implementation.

Gankra reviewed Jul 13, 2014
View reviewed changes

Change TreeMultiset len() to return the number of occurrences. The pr…

b527a30

…evious version returned the number of distinct values.

nham added 3 commits July 14, 2014 00:23

Make TreeMultiset.extend() use insert_one

48add65

Add sum operation to TreeMultiset

4592fdc

Add test_from_iter to TreeMultiset

d7c2a91

treeman mentioned this pull request Jul 14, 2014

Implement a multimap #15669

Closed

nham added 3 commits July 16, 2014 14:59

Rename [insert|remove]_one to [insert|remove] and [insert|remove] to …

ac30b87

…[insert|remove]_many

Make TreeMultiset implement Hash

8bdcb1b

Remove the Multiset and MutableMultiset traits.

3e7c70b

The collections crate is still in flux, so we are holding off on deciding the design of this trait.

Implement lower_bound and upper_bound

dce9de8

Remove trailing whitespace, duplicate function definition.

05aa1f3

Gankra reviewed Jul 29, 2014
View reviewed changes

pczarn reviewed Jul 29, 2014
View reviewed changes

Inline a bunch of methods. Change one 0u -> 0

3af7a28

nham closed this Aug 9, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libcollections: Add a Multiset data structure. #15623

libcollections: Add a Multiset data structure. #15623

nham commented Jul 11, 2014

Gankra commented Jul 12, 2014

Gankra Jul 12, 2014

nham Jul 12, 2014

Gankra Jul 14, 2014

nham Jul 15, 2014

Gankra Jul 13, 2014

nham Jul 13, 2014

Gankra commented Jul 14, 2014

nham commented Jul 15, 2014

alexcrichton commented Jul 16, 2014

nham commented Jul 16, 2014

nham commented Jul 22, 2014

Gankra Jul 29, 2014

nham Jul 31, 2014

Gankra commented Jul 29, 2014

Gankra Jul 29, 2014

pczarn Jul 29, 2014

nham Jul 31, 2014

pczarn Jul 31, 2014

nham Aug 6, 2014

nham commented Aug 9, 2014

Gankra commented Aug 9, 2014

nham commented Aug 9, 2014

Gankra commented Aug 9, 2014

nham commented Aug 9, 2014

Gankra commented Aug 9, 2014

nham commented Aug 9, 2014

Gankra commented Aug 9, 2014

libcollections: Add a Multiset data structure. #15623

libcollections: Add a Multiset data structure. #15623

Conversation

nham commented Jul 11, 2014

Gankra commented Jul 12, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gankra commented Jul 14, 2014

nham commented Jul 15, 2014

alexcrichton commented Jul 16, 2014

nham commented Jul 16, 2014

nham commented Jul 22, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gankra commented Jul 29, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nham commented Aug 9, 2014

Gankra commented Aug 9, 2014

nham commented Aug 9, 2014

Gankra commented Aug 9, 2014

nham commented Aug 9, 2014

Gankra commented Aug 9, 2014

nham commented Aug 9, 2014

Gankra commented Aug 9, 2014