Add Iterators #41

iwburns · 2017-02-02T03:51:26Z

Fixes #40

clean up a few use blocks

iwburns · 2017-02-02T03:53:04Z

Still need to add docs and doc tests for this

iwburns · 2017-02-02T04:19:34Z

@Drakulix if you don't mind I'd love to get your thoughts on my approach here. It's still a WiP, of course, but any thoughts would be appreciated.

Drakulix

Looks quite good to me!

I am kinda disliking how Tree get more "crowded" with functions that should belong to Node. Calling node.anchestors() would be far more intuitive. I undestand, why this is not possible right now, but I am really thinking, if a Node should not contain a reference to it's Tree at some point.

But thats a subject for a new ussue, the changes overall fit the current api and look mostly good. I have added some comments on stuff, that hit my eye.

Drakulix · 2017-02-02T20:36:43Z

src/tree/iterators.rs

+
+                self.node_id = Some(parent_id.clone());
+
+                return Some(parent);


Spacing looks weird here.

I agree, will fix that shortly.

Drakulix · 2017-02-02T20:38:55Z

src/tree/iterators.rs

+impl<'a, T> IteratorNew<'a, T, ChildrenIds<'a>> for ChildrenIds<'a> {
+    fn new(tree: &'a Tree<T>, node_id: NodeId) -> ChildrenIds<'a> {
+        ChildrenIds { child_ids: tree.get_unsafe(&node_id).children().as_slice().iter() }
+    }


Couldn't you just return the Iterator directly?

No particular reason to be honest.

So (if I'm understanding you correctly), basically the other way to do this would be to have
tree.children_ids(...)
just do this:
return self.get_unsafe(...).children().as_slice().iter()
(or whatever the equivalent code that type-checks properly would be) and then not have a ChildrenIds struct at all, correct?

Yep.
What you really want here is this nightly feature: rust-lang/rfcs#1522

But since there is no real reason, this crate should not work on stable, on second thought, it might be better to hide the underlying Iterator, so you can more easily change it, until RFC 1522 has been stabilized.

I think I see what you're saying here, but I'm not sure I'm following how I would

hide the underlying Iterator, so you can more easily change it, until RFC 1522 has been stabilized.

Do you mind clarifying?

Hiding would be essentially what you do now. Not exposing the type but instead wrapping it in your struct.
I basically changed my mind, thinking about what guarantees we want to make in the API.
You basically want to express, that we return an Iterator, not what kind of Iterator. Hiding it by wrapping makes it easier to change it later without the need to break the API.

Ah, I see what you mean. Sorry for the confusion, and yes I agree!

Drakulix · 2017-02-02T20:40:56Z

src/tree/mod.rs

+trait IteratorNew<'a, T, I> {
+    fn new(tree: &'a Tree<T>, node_id: NodeId) -> I;
+}
+


Any particular reason, why you have this trait? Any reason not to make these new functions "normal" struct methods?

If I'm not mistaken, this is the only way to make sure that the new(...) function is available the tree module, but not have it available any higher up than that.

Essentially I just wanted to be extra safe and not allow the library user to call new(...) on anything in the iterators module directly and this is the only pattern I could think of that would provide that behavior.

Because the Iterators are in a lower module, I think you would not need to make them public.
But of course, if this is about visibility, it is understandable.

I'll see if I can re-visit this so that I don't have to do the trait workaround. I just don't want them public from the library perspective. Internally, I don't really care where they are (although I was trying to keep them in their own file because the tree module is getting large).

I would try if making them non-public "normal" methods does compile and otherwise just revert back to this solution.

iwburns · 2017-02-02T21:28:35Z

Yeah, I agree it seems odd that you have to ask the Tree for these iterators, but I'd have to think through the implications of having Nodes know which Tree they belong to.

Drakulix · 2017-02-02T22:10:52Z

Yeah, I agree it seems odd that you have to ask the Tree for these iterators, but I'd have to think through the implications of having Nodes know which Tree they belong to.

Yes, this would need to be another issue. Just wanted to bring that up, instead of completely ignoring it.

iwburns · 2017-02-09T19:04:45Z

src/tree/iterators.rs

+            data: Vec::with_capacity(tree.nodes.capacity()),
+        };
+
+        traversal.process_nodes(node_id);


@Drakulix I'm not 100% happy with this because it basically forces all of the computation to be run upon creating the Iterator, regardless of whether or not the user ends up using the whole iterator.

However, I couldn't come up with a good way to do this incrementally as next() is called. (I found a way for PreOrder but PostOrder has stumped me). Do you have any thoughts on this?

It's certainly tricky, but I think not impossible, but maybe not worth the effort.

I guess you would need to store a stack inside the Iterator. Entries would be a node + the current index of its children and then start with index 0 and the root node. Then build up the stack on a next call until you have the next element and so on. you are done with a node and pop it from the stack, when the index would go out of bounds.

That should be the minimal amount of state required. you could probably also just store the current id + index, but then you would need to back-track via parent() and find out the child's position and that sounds unnecessary to me.

Yeah, good points. That would probably be the way to go. I'm probably going to leave it for now and create an issue for later. Thanks for taking a look!

iwburns · 2017-03-08T17:26:32Z

@Drakulix I know it's been a while, but I'd like your opinion again if you have the time.

I'm mostly happy with this as a v1.1.0 release, but I'm wondering if I should add a Level-Order Traversal as well. It seems like it might be useful enough to include, right?

Also, all of the traversal iterators currently iterate directly over the Nodes themselves, but I'm wondering if it would be nice to also have a NodeId version of each of them (just like the child and ancestor iterators).

Drakulix · 2017-03-08T17:50:01Z

Looks good to me, except for that process_node call we already talked about. But I would most likely go the easy route as well, this is difficult to optimize and get right.

I'm mostly happy with this as a v1.1.0 release, but I'm wondering if I should add a Level-Order Traversal as well. It seems like it might be useful enough to include, right?

I even think, I have a use case for this in my window manager. So yes, it is definitely useful, but if it takes too much time now, I would not hold back 1.1.0 for too long and just push that to 1.2.0.

Also, all of the traversal iterators currently iterate directly over the Nodes themselves, but I'm wondering if it would be nice to also have a NodeId version of each of them (just like the child and ancestor iterators).

I cannot think of many use cases for this and generally this should be avoided, because you will be dealing with Ids and therefor a bunch of unwraps or excepts again. Having only Node iterators is a feature not a short-coming in my eyes.

iwburns · 2017-03-08T18:06:43Z

Looks good to me, except for that process_node call we already talked about. But I would most likely go the easy route as well, this is difficult to optimize and get right.

Yeah, I'd love to get it right and fast the first time, but I'd rather not deal with it right now (I've let this sit here much longer than I wanted to originally). Plus it should be something that can be optimized later without breaking anything.

I even think, I have a use case for this in my window manager. So yes, it is definitely useful, but if it takes too much time now, I would not hold back 1.1.0 for too long and just push that to 1.2.0.

It shouldn't be too hard to do, but the implementation will be a naive one (similar to the process_node situation) for now. I'm fine with leaving those as future optimizations so that I can get this functionality out there sooner rather than later.

I cannot think of many use cases for this and generally this should be avoided, because you will be dealing with Ids and therefor a bunch of unwraps or excepts again. Having only Node iterators is a feature not a short-coming in my eyes.

I mostly agree, but I'm worried there are use-cases out there that might require NodeId iterators instead of Node iterators that I'm just not aware of. Maybe that's not something that I should be worrying too much over though.

Drakulix · 2017-03-08T18:30:09Z

I mostly agree, but I'm worried there are use-cases out there that might require NodeId iterators instead of Node iterators that I'm just not aware of. Maybe that's not something that I should be worrying too much over though.

Exactly. In the worst case you get a feature request half a year later.

also, tweak testing to be more clear and precise

iwburns · 2017-03-09T01:57:49Z

@Drakulix I've switched things up a bit (nothing too crazy, but I forgot I had some changes on a different machine that cleaned up one of the iterators a bit). I've added the LevelOrderTraversal as well.

Do you mind taking a look when you have a chance?

Edit: since this comment I was able to get rid of the process_nodes call in LevelOrderTraversal.

Drakulix · 2017-03-09T14:47:44Z

Just took a look over everything. Looks good to me :)

Edit: since this comment I was able to get rid of the process_nodes call in LevelOrderTraversal.

Including this. This makes switching to this Iterator even more valuable for me.

iwburns · 2017-03-09T16:33:57Z

Including this. This makes switching to this Iterator even more valuable for me.

That's great to hear. I also simplified the PreOrderTraversal a bit just now. I'm sorry to keep asking, but if you don't mind I'd love another set of eyes on it. This is the last set of changes that I wanted to make before merging this.

Drakulix · 2017-03-09T17:00:51Z

I also simplified the PreOrderTraversal a bit just now. I'm sorry to keep asking, but if you don't mind I'd love another set of eyes on it.

No worries. Again looks fine. :)

iwburns added 2 commits February 1, 2017 19:23

add Ancestors and Children iterators

69e7d55

add Id version of iterators.

c198c6a

clean up a few use blocks

iwburns changed the title ~~Add Iterators~~ WiP: Add Iterators Feb 2, 2017

iwburns added the enhancement label Feb 2, 2017

run cargo fmt

4bc1c44

iwburns added 2 commits February 1, 2017 22:06

add some docs for Iterator structs

a2008f1

fix bad use block

3ebd6b2

iwburns added 2 commits February 1, 2017 22:40

add more docs

20d97c4

WiP: add a few more tests

a28c680

Drakulix reviewed Feb 2, 2017

View reviewed changes

iwburns added 4 commits February 2, 2017 16:48

remove some extra spaces

06cc161

add PreOrderTraversal (still need docs for it)

c60c59e

add PostOrderTraversal (still need docs for it)

61d3b79

fix formatting

e543eb0

iwburns commented Feb 9, 2017

View reviewed changes

iwburns added 9 commits February 9, 2017 13:18

update basic example.

89768e7

add struct docs

42a68c1

add more docs.

5b31ea6

run cargo fmt

20a4a68

add more iterator tests

c661f06

remove unnecessary lifetime

a5eb1c6

get rid of extra usize and bounds checking

ce51cad

update docs example

902a219

version bump

6b3713e

iwburns added 4 commits March 8, 2017 18:44

Merged branch iterators into iterators

390e07c

add LevelOrderTraversal

c8b7570

also, tweak testing to be more clear and precise

fix formatting

7e913c9

switch to storing a vec_deque::IntoIter

943a53c

switch to owning the VecDeque and get rid of the process_nodes call

593fae0

iwburns added 2 commits March 9, 2017 09:36

simplify PreOrderTraversal memory layout and next()

ebc54d6

fix formatting

fcea716

iwburns changed the title ~~WiP: Add Iterators~~ Add Iterators Mar 9, 2017

iwburns merged commit 02612f1 into master Mar 9, 2017

iwburns deleted the iterators branch March 9, 2017 23:42

Drakulix mentioned this pull request Mar 9, 2017

Update README #42

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Iterators #41

Add Iterators #41

iwburns commented Feb 2, 2017 •

edited

Loading

iwburns commented Feb 2, 2017

iwburns commented Feb 2, 2017

Drakulix left a comment •

edited

Loading

Drakulix Feb 2, 2017

iwburns Feb 2, 2017

Drakulix Feb 2, 2017

iwburns Feb 2, 2017

Drakulix Feb 2, 2017

iwburns Feb 2, 2017

Drakulix Feb 3, 2017

iwburns Feb 3, 2017

Drakulix Feb 2, 2017

iwburns Feb 2, 2017

Drakulix Feb 2, 2017

iwburns Feb 2, 2017

Drakulix Feb 3, 2017

iwburns commented Feb 2, 2017

Drakulix commented Feb 2, 2017

iwburns Feb 9, 2017 •

edited

Loading

Drakulix Feb 9, 2017

iwburns Feb 10, 2017

iwburns commented Mar 8, 2017

Drakulix commented Mar 8, 2017

iwburns commented Mar 8, 2017 •

edited

Loading

Drakulix commented Mar 8, 2017

iwburns commented Mar 9, 2017 •

edited

Loading

Drakulix commented Mar 9, 2017 •

edited

Loading

iwburns commented Mar 9, 2017

Drakulix commented Mar 9, 2017

Add Iterators #41

Add Iterators #41

Conversation

iwburns commented Feb 2, 2017 • edited Loading

iwburns commented Feb 2, 2017

iwburns commented Feb 2, 2017

Drakulix left a comment • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwburns commented Feb 2, 2017

Drakulix commented Feb 2, 2017

iwburns Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwburns commented Mar 8, 2017

Drakulix commented Mar 8, 2017

iwburns commented Mar 8, 2017 • edited Loading

Drakulix commented Mar 8, 2017

iwburns commented Mar 9, 2017 • edited Loading

Drakulix commented Mar 9, 2017 • edited Loading

iwburns commented Mar 9, 2017

Drakulix commented Mar 9, 2017

iwburns commented Feb 2, 2017 •

edited

Loading

Drakulix left a comment •

edited

Loading

iwburns Feb 9, 2017 •

edited

Loading

iwburns commented Mar 8, 2017 •

edited

Loading

iwburns commented Mar 9, 2017 •

edited

Loading

Drakulix commented Mar 9, 2017 •

edited

Loading