-
Notifications
You must be signed in to change notification settings - Fork 545
Pov exercise #2077
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
base: main
Are you sure you want to change the base?
Pov exercise #2077
Changes from 5 commits
213dfec
721c5bf
9e6b18b
a511858
623f98c
19ea634
ffbc4b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ tmp | |
/bin/configlet | ||
exercises/*/*/Cargo.lock | ||
clippy.log | ||
.idea | ||
.vscode | ||
.prob-spec | ||
problem-specifications |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
# Instructions | ||
|
||
Reparent a tree on a selected node. | ||
|
||
A [tree][wiki-tree] is a special type of [graph][wiki-graph] where all nodes are connected but there are no cycles. | ||
That means, there is exactly one path to get from one node to another for any pair of nodes. | ||
|
||
This exercise is all about re-orientating a tree to see things from a different point of view. | ||
For example family trees are usually presented from the ancestor's perspective: | ||
|
||
```text | ||
+------0------+ | ||
| | | | ||
+-1-+ +-2-+ +-3-+ | ||
| | | | | | | ||
4 5 6 7 8 9 | ||
``` | ||
|
||
But there is no inherent direction in a tree. | ||
The same information can be presented from the perspective of any other node in the tree, by pulling it up to the root and dragging its relationships along with it. | ||
So the same tree from 6's perspective would look like: | ||
|
||
```text | ||
6 | ||
| | ||
+-----2-----+ | ||
| | | ||
7 +-----0-----+ | ||
| | | ||
+-1-+ +-3-+ | ||
| | | | | ||
4 5 8 9 | ||
``` | ||
|
||
This lets us more simply describe the paths between two nodes. | ||
So for example the path from 6-9 (which in the first tree goes up to the root and then down to a different leaf node) can be seen to follow the path 6-2-0-3-9. | ||
|
||
This exercise involves taking an input tree and re-orientating it from the point of view of one of the nodes. | ||
|
||
[wiki-graph]: https://en.wikipedia.org/wiki/Tree_(graph_theory) | ||
[wiki-tree]: https://en.wikipedia.org/wiki/Graph_(discrete_mathematics) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
/target | ||
Cargo.lock |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
{ | ||
"authors": [ | ||
"devcyjung" | ||
], | ||
"files": { | ||
"solution": [ | ||
"src/lib.rs", | ||
"Cargo.toml" | ||
], | ||
"test": [ | ||
"tests/pov.rs" | ||
], | ||
"example": [ | ||
".meta/example.rs" | ||
] | ||
}, | ||
"blurb": "Reparent a graph on a selected node.", | ||
"source": "Adaptation of exercise from 4clojure", | ||
"source_url": "https://github.com/oxalorg/4ever-clojure" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,96 @@ | ||
use std::fmt::Debug; | ||
use std::ops::{Deref, DerefMut}; | ||
|
||
#[derive(Clone, Debug, PartialEq)] | ||
pub struct Tree<T: Clone + Debug + PartialEq> { | ||
label: T, | ||
children: Vec<Box<Tree<T>>>, | ||
} | ||
|
||
impl<T: Clone + Debug + PartialEq> Tree<T> { | ||
#[must_use] | ||
devcyjung marked this conversation as resolved.
Show resolved
Hide resolved
|
||
pub fn new(label: T) -> Self { | ||
Self { | ||
label, | ||
children: Default::default(), | ||
} | ||
} | ||
|
||
#[must_use] | ||
pub fn with_children(label: T, children: Vec<Self>) -> Self { | ||
Self { | ||
label, | ||
children: children.into_iter().map(Box::new).collect(), | ||
} | ||
} | ||
|
||
#[must_use] | ||
pub fn get_label(&self) -> T { | ||
self.label.clone() | ||
} | ||
|
||
#[must_use] | ||
pub fn get_children(&self) -> Vec<&Self> { | ||
self.children.iter().map(Box::deref).collect() | ||
} | ||
|
||
#[must_use] | ||
pub fn pov_from(&self, from: &T) -> Option<Self> { | ||
// list of (child, parent, child's index in parent.children) | ||
let mut lookup = vec![(self, None, None)]; | ||
let mut stack = vec![self]; | ||
while let Some(parent) = stack.pop() { | ||
if &parent.label == from { | ||
return self.reparent(parent, lookup.as_slice()).into(); | ||
} | ||
lookup.extend( | ||
parent | ||
.children | ||
.iter() | ||
.map(Box::deref) | ||
.enumerate() | ||
.map(|(i, child)| (child, Some(parent), Some(i))), | ||
); | ||
stack.extend(parent.children.iter().map(Box::deref)); | ||
} | ||
None | ||
} | ||
|
||
// lookup is list of (child, parent, child's index in parent.children) | ||
#[must_use] | ||
fn reparent(&self, parent: &Self, lookup: &[(&Self, Option<&Self>, Option<usize>)]) -> Self { | ||
let mut new_root = parent.clone(); | ||
let mut current = parent; | ||
let mut clone_mut = &mut new_root; | ||
let find_parent = |child| lookup.iter().find(|(c, _p, _i)| *c == child); | ||
while let Some(&(_, Some(parent), Some(index))) = find_parent(current) { | ||
let mut parent_clone = parent.clone(); | ||
parent_clone.children.swap_remove(index); | ||
clone_mut.children.push(Box::new(parent_clone)); | ||
current = parent; | ||
let new_box = clone_mut | ||
.children | ||
.last_mut() | ||
.expect("We just inserted node, this is not empty"); | ||
clone_mut = new_box.deref_mut(); | ||
} | ||
new_root | ||
} | ||
|
||
#[must_use] | ||
pub fn path_to(&self, from: &T, to: &T) -> Option<Vec<T>> { | ||
if from != &self.label { | ||
return self.pov_from(from).and_then(|pov| pov.path_to(from, to)); | ||
} | ||
if to == &self.label { | ||
return Some(vec![self.label.clone()]); | ||
} | ||
for child in self.children.iter() { | ||
if let Some(mut path) = child.path_to(&child.label, to) { | ||
path.insert(0, self.label.clone()); | ||
return Some(path); | ||
} | ||
} | ||
None | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
{%- macro render_tree(tree) -%} | ||
{%- if tree.children -%} | ||
Tree::with_children( | ||
"{{ tree.label }}".to_string(), | ||
vec![ | ||
{%- for child in tree.children -%} | ||
{{ self::render_tree(tree=child) }}, | ||
{%- endfor -%} | ||
], | ||
) | ||
{%- else -%} | ||
Tree::new("{{ tree.label }}".to_string()) | ||
{%- endif -%} | ||
{%- endmacro -%} | ||
|
||
{%- macro render_vec(values) -%} | ||
vec![ | ||
{%- for value in values -%} | ||
"{{ value }}".to_string(), | ||
{%- endfor -%} | ||
] | ||
{%- endmacro -%} | ||
|
||
{% for test_group in cases %} | ||
/// {{ test_group.description }} | ||
mod {{ test_group.cases[0].property | make_ident }} { | ||
use pov::*; | ||
use pretty_assertions::assert_eq; | ||
|
||
{% for test in test_group.cases %} | ||
#[test] | ||
#[ignore] | ||
fn {{ test.description | make_ident }}() { | ||
let input = {{ self::render_tree(tree=test.input.tree) }}; | ||
let from = "{{ test.input.from }}".to_string(); | ||
{%- if test.property == "fromPov" -%} | ||
let result = input.pov_from(&from); | ||
{%- if not test.expected -%} | ||
let expected: Option<Tree<String>> = None; | ||
{%- else -%} | ||
let expected = Some({{ self::render_tree(tree=test.expected) }}); | ||
{%- endif -%} | ||
assert_eq!(result.map(|t| crate::test_util::tree_to_sorted(&t)), expected.map(|t| crate::test_util::tree_to_sorted(&t))); | ||
{%- elif test.property == "pathTo" -%} | ||
let to = "{{ test.input.to }}".to_string(); | ||
let result = input.path_to(&from, &to); | ||
{%- if not test.expected -%} | ||
let expected: Option<Vec<String>> = None; | ||
{%- else -%} | ||
let expected = Some({{ self::render_vec(values=test.expected) }}); | ||
{%- endif -%} | ||
assert_eq!(result, expected); | ||
{%- else -%} | ||
Invalid property: {{ test.property }} | ||
{%- endif -%} | ||
} | ||
{% endfor %} | ||
} | ||
{% endfor %} | ||
|
||
mod test_util { | ||
use pov::*; | ||
pub fn tree_to_sorted<T: Ord + Clone + std::fmt::Debug>(tree: &Tree<T>) -> Tree<T> { | ||
let mut children = tree.get_children(); | ||
children.sort_unstable_by_key(|child| child.get_label()); | ||
Tree::with_children(tree.get_label(), children.iter().map(|c| tree_to_sorted(c)).collect()) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
# This is an auto-generated file. | ||
# | ||
# Regenerating this file via `configlet sync` will: | ||
# - Recreate every `description` key/value pair | ||
# - Recreate every `reimplements` key/value pair, where they exist in problem-specifications | ||
# - Remove any `include = true` key/value pair (an omitted `include` key implies inclusion) | ||
# - Preserve any other key/value pair | ||
# | ||
# As user-added comments (using the # character) will be removed when this file | ||
# is regenerated, comments can be added via a `comment` key. | ||
|
||
[1b3cd134-49ad-4a7d-8376-7087b7e70792] | ||
description = "Reroot a tree so that its root is the specified node. -> Results in the same tree if the input tree is a singleton" | ||
|
||
[0778c745-0636-40de-9edd-25a8f40426f6] | ||
description = "Reroot a tree so that its root is the specified node. -> Can reroot a tree with a parent and one sibling" | ||
|
||
[fdfdef0a-4472-4248-8bcf-19cf33f9c06e] | ||
description = "Reroot a tree so that its root is the specified node. -> Can reroot a tree with a parent and many siblings" | ||
|
||
[cbcf52db-8667-43d8-a766-5d80cb41b4bb] | ||
description = "Reroot a tree so that its root is the specified node. -> Can reroot a tree with new root deeply nested in tree" | ||
|
||
[e27fa4fa-648d-44cd-90af-d64a13d95e06] | ||
description = "Reroot a tree so that its root is the specified node. -> Moves children of the new root to same level as former parent" | ||
|
||
[09236c7f-7c83-42cc-87a1-25afa60454a3] | ||
description = "Reroot a tree so that its root is the specified node. -> Can reroot a complex tree with cousins" | ||
|
||
[f41d5eeb-8973-448f-a3b0-cc1e019a4193] | ||
description = "Reroot a tree so that its root is the specified node. -> Errors if target does not exist in a singleton tree" | ||
|
||
[9dc0a8b3-df02-4267-9a41-693b6aff75e7] | ||
description = "Reroot a tree so that its root is the specified node. -> Errors if target does not exist in a large tree" | ||
|
||
[02d1f1d9-428d-4395-b026-2db35ffa8f0a] | ||
description = "Given two nodes, find the path between them -> Can find path to parent" | ||
|
||
[d0002674-fcfb-4cdc-9efa-bfc54e3c31b5] | ||
description = "Given two nodes, find the path between them -> Can find path to sibling" | ||
|
||
[c9877cd1-0a69-40d4-b362-725763a5c38f] | ||
description = "Given two nodes, find the path between them -> Can find path to cousin" | ||
|
||
[9fb17a82-2c14-4261-baa3-2f3f234ffa03] | ||
description = "Given two nodes, find the path between them -> Can find path not involving root" | ||
|
||
[5124ed49-7845-46ad-bc32-97d5ac7451b2] | ||
description = "Given two nodes, find the path between them -> Can find path from nodes other than x" | ||
|
||
[f52a183c-25cc-4c87-9fc9-0e7f81a5725c] | ||
description = "Given two nodes, find the path between them -> Errors if destination does not exist" | ||
|
||
[f4fe18b9-b4a2-4bd5-a694-e179155c2149] | ||
description = "Given two nodes, find the path between them -> Errors if source does not exist" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
[package] | ||
name = "pov" | ||
version = "0.1.0" | ||
edition = "2024" | ||
|
||
# Not all libraries from crates.io are available in Exercism's test runner. | ||
# The full list of available libraries is here: | ||
# https://github.com/exercism/rust-test-runner/blob/main/local-registry/Cargo.toml | ||
[dependencies] | ||
|
||
[dev-dependencies] | ||
pretty_assertions = "1.4.1" | ||
devcyjung marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,34 @@ | ||||||
use std::fmt::Debug; | ||||||
|
||||||
#[derive(Clone, Debug, PartialEq)] | ||||||
pub struct Tree<T: Clone + Debug + PartialEq> { | ||||||
_remove_this: std::marker::PhantomData<T>, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can remove the prefix underscore here. This is why CI is failing. The compiler actually won't complain about unused code, because it knows that fields of type |
||||||
} | ||||||
Comment on lines
+4
to
+6
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The exercise is not primarily about generics, so what do you think about dropping them and using something like this for the label: // In production code, this would be a type parameter on `Tree`.
pub type Label = &'static str; I'm not 100% sure, maybe it's dumbing it down too much. But right now I'm not seeing the value of making the user implement this with generics. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I initially started with non-generic Tree, but along the way I realized that making this non-generic doesn't really make the exercise any simpler. So I added generic. It also has expected trait bound of PartialEq and Clone, so users can expect where their solution should be directed to. Also with the non-generic Tree I spent a lot of time should I make this return type impl AsRef or just String, or Cow<'_, str>, etc There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please don't mark threads as resolved unless it's actually resolved, i.e. there is no reason for me or you to look at it again.
What return type are you referring to? I would think you could use I think generics do make the exercise more complicated. It's not guaranteed that everyone who solves this exercise is already experienced with them. That's why I asked for the specific value they add. Unless generics add value, I would prefer to remove them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Answering from this comment here on the topic of removing / keeping generics:
I don't think generics must necessarily be a big part of the exercise just because it's about a data structure. I think the essence of this exercise is how to handle
I do not think this is the ultimate challenge for data structure design. I'm much more interested in a focused exercise about handling
I see this as another argument against generics. With the current bounds, users cannot use a
I think the fact that the trait bounds are either too restrictive or too loose for any given implementation is a pretty good argument against generics. In addition to that, I maintain that it's needless complexity that distracts from the learning purpose of the exercise. As commented elsewhere, I now see an argument in favor of generics. Please use the following bounds: |
||||||
|
||||||
impl<T: Clone + Debug + PartialEq> Tree<T> { | ||||||
pub fn new(label: T) -> Self { | ||||||
todo!("Implement a function that creates a new Tree with given {label:?}"); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} | ||||||
|
||||||
pub fn with_children(label: T, children: Vec<Self>) -> Self { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like a builder method, but it isn't. I think this is more idiomatic: pub fn with_children(self, children: Vec<Self>) -> Self { todo!() } Users of this API can then construct the tree with The standard implementation would be to make Now hat I think about it, I think there's an even better option: pub fn with_child(self, child: Self) -> Self { todo!() } Then we just call that method once per child. Users will have to append the child to their data structure of choice. If we already give users a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok in that case the testcases will be a lot longer, but if you are ok with that I'll do so There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. with_children naming came from the Vec::with_capacity But either way I'm good with that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, making the tests longer is not that big of a deal. I like the last option because it doesn't prime the user to use a specific data structure. Another way to do that would be to have a parameter There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'm leaning to with_children(self, Vec) idea First, with_child is unclear for student. We are not dealing with a linked list or binary tree. It's a n-ary tree and that should be clear from the methods. I want to avoid generic method if possible. Generic itself is not alien to students. All the basic of basic types are generic. Vec, Box, HashMap, Result. etc. Such as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In that case, go with one of these two options:
I'm against |
||||||
todo!("Implement a function that creates a new Tree with given {label:?} and {children:?}"); | ||||||
} | ||||||
|
||||||
pub fn get_label(&self) -> T { | ||||||
todo!("Implement getter for label."); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} | ||||||
|
||||||
pub fn get_children(&self) -> Vec<&Self> { | ||||||
devcyjung marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
todo!("Implement getter for children."); | ||||||
} | ||||||
|
||||||
pub fn pov_from(&self, from: &T) -> Option<Self> { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. quoting from another comment:
I hadn't noticed that yet, but I think the solution should definitely use a mutable approach. Without that, marking the exercise as difficult makes no sense. Users should be encouraged to reuse allocations whenever reasonably possible. Rust is a performance-oriented language. That doesn't mean performance has to be the focus of everything, but we shouldn't design our APIs in a way that forces users into an inefficient implementation. I propose: fn pov_from(tree: &mut Option<Box<Tree>>, from: &T) { todo!() } If desired, this can be split in two types Now I actually see an argument in favor of generics... With generics, we could add a special test case with a type that doesn't implement There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @senekor Can you please explain why tree is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, I don't know. I think last time I had to implement something like this, using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @senekor ok I'll implement as close to what you suggested, do you expect it will be merged soon by then? It will take a day or two for me I think since the API is totally different There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I cannot say in advance when this will be merged. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At this point, please don't mark any conversations as resolved, ever. I need the resolved state to keep track of what has been completed and what hasn't. I will mark conversations as resolved when I do a review and find that I won't need to review it again in the future. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apologies on the delay! I'm working hard on the solution, will commit today There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry again, I'm failing 1 case again and again, I'll make sure to push by today There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No worries, there is no pressure. This is an open source project, people have lives and stuff to do, nobody is expecting you to finish something by a specific deadline! Take it easy and simply rerequest a review once you're done! 🙂 |
||||||
todo!("Implement a function that reparents Tree with {from:?} as root."); | ||||||
} | ||||||
|
||||||
pub fn path_to(&self, from: &T, to: &T) -> Option<Vec<T>> { | ||||||
todo!( | ||||||
"Implement a function that returns the list of labels in the shortest path from {from:?} to {to:?}" | ||||||
); | ||||||
} | ||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not yet sure about the difficulty. I'm not asking to change it yet, it's just a note for me so I don't forget to think it through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't mark things as resolved without a comment, I posted this so I don't forget about it. I'm not sure if the difficulty should be 10 or maybe reduced to 7, I'll have to compare with the other exercises.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the list of difficult exercises on the rust track:
With the exception of ocr-numbers and rectangles, all of these are more difficult than
pov
in my opinion. I would actually consider downgrading ocr-numbers and rectangles to medium, I don't see what's difficult about them.A comparable exercise is simple-linked-list, it also involves
Option<Box<T>>
. But it's clearly easier, it doesn't involve any weird manipulation of the tree.Looking over all the other medium exercises,
pov
is clearly more difficult than most of them. Few if any of the medium exercises actually require dealing with Rust's ownership system in a meaningful sense likepov
. So, while I still think it's on the lower end of difficult, I'm okay with keeping it there.