Skip to content

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ tmp
/bin/configlet
exercises/*/*/Cargo.lock
clippy.log
.idea
.vscode
.prob-spec
problem-specifications
9 changes: 9 additions & 0 deletions config.json
Original file line number Diff line number Diff line change
Expand Up @@ -1572,6 +1572,15 @@
"lists",
"unsafe"
]
},
{
"slug": "pov",
"name": "POV",
"uuid": "bf7b7309-3d34-4893-a584-f7742502e012",
"practices": [],
"prerequisites": [],
"difficulty": 10,
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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:

  • parallel-letter-frequency
  • macros
  • poker
  • forth
  • ocr-numbers
  • react
  • circular buffer
  • rectangles
  • xorcism
  • dominoes
  • doubly-linked-list

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 like pov. So, while I still think it's on the lower end of difficult, I'm okay with keeping it there.

"topics": []
}
],
"foregone": [
Expand Down
41 changes: 41 additions & 0 deletions exercises/practice/pov/.docs/instructions.md
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)
2 changes: 2 additions & 0 deletions exercises/practice/pov/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
/target
Cargo.lock
20 changes: 20 additions & 0 deletions exercises/practice/pov/.meta/config.json
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"
}
96 changes: 96 additions & 0 deletions exercises/practice/pov/.meta/example.rs
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]
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
}
}
68 changes: 68 additions & 0 deletions exercises/practice/pov/.meta/test_template.tera
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())
}
}
55 changes: 55 additions & 0 deletions exercises/practice/pov/.meta/tests.toml
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"
12 changes: 12 additions & 0 deletions exercises/practice/pov/Cargo.toml
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"
34 changes: 34 additions & 0 deletions exercises/practice/pov/src/lib.rs
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>,
Copy link
Contributor

Choose a reason for hiding this comment

The 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 PhantomData are intended to never be read.

}
Comment on lines +4 to +6
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Author

@devcyjung devcyjung Jul 3, 2025

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

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

What return type are you referring to? I would think you could use Label or &'static str in any position where you're now using T.

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Answering from this comment here on the topic of removing / keeping generics:

Also designing the data structure itself is a big part of this exercise. That's why I like the generic idea. Data structure is the best use case of 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 Option<Box<T>> with mutation. The choice between Vec and HashSet is neither important nor interesting.

If this is the ultimate challenge for data structure design, I think we should go for a proper generic support

I do not think this is the ultimate challenge for data structure design. I'm much more interested in a focused exercise about handling Option<Box<T>> with mutation.

The Trait bound that I chose didn't come from own my implementation. If that were the case I would have added Hash so I can use HashMap for trail.

I see this as another argument against generics. With the current bounds, users cannot use a HashSet for their implementation. They have to further restrict the bounds with Eq + Hash.

There's no reason not to go generic imo.

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: T: Debug + Eq + Hash. This will allow us to add a test case with a type that doesn't implement Clone ensuring users don't cheat by cloning nodes.


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:?}");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
todo!("Implement a function that creates a new Tree with given {label:?}");
todo!("Create a new Tree with the given {label:?}");

}

pub fn with_children(label: T, children: Vec<Self>) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

The 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 Tree::new(label).with_children(vec![]).

The standard implementation would be to make self mutable and overwrite the children. Vec doesn't allocate when empty, so there's no perf downside.


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 Vec in the constructor, we push them to use a Vec internally too, although a HashSet might also be a good choice.

Copy link
Author

Choose a reason for hiding this comment

The 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

Copy link
Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 children: impl Iterator<Item = Self>. That can be collected into any data structure, vec or hash set. Either way is fine by me.

Copy link
Author

@devcyjung devcyjung Jul 3, 2025

Choose a reason for hiding this comment

The 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.
Second impl Iterator is basically I: Iterator if it's parameter. This also heavily deviates from the main idea of the practice. with_children and new are just setups so that we get the the main meat which is pov_from and path_to

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.
What's alien to most students is generic method

Such as fn <I,V,F>....... and I agree this is unnecessary complication for this exercise. This fits in exercise like List Op, but not here.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, go with one of these two options:

fn with_child(self, child: Self) -> Self {}
fn add_child(&mut self, child: Self) {}

I'm against with_children(self, Vec) because it gently pushes people toward a specific implementation.

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.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
todo!("Implement getter for label.");
todo!("Return the tree's label.");

}

pub fn get_children(&self) -> Vec<&Self> {
todo!("Implement getter for children.");
}

pub fn pov_from(&self, from: &T) -> Option<Self> {
Copy link
Contributor

Choose a reason for hiding this comment

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

quoting from another comment:

Clone is necessary because we want them to make immutable solution. It's in the canonical json spec comments that language tracks can choose to enforce immutability, and in Rust making mutable solution is arguably more complex anyways.

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 Tree and Node, where the tree only holds a Option<Box<Node>> and the node is where the action happens in terms of the data structure.

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 Clone, to make sure users don't add the Clone bound themselves to cheat.

Copy link
Author

@devcyjung devcyjung Jul 4, 2025

Choose a reason for hiding this comment

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

@senekor Can you please explain why tree is Option? How about &mut Box<Tree>, and instead return a bool to mark absence of match

Copy link
Contributor

Choose a reason for hiding this comment

The 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 Option was the way to go. But I'm not sure, could be different in this case. If &mut Box<Tree> works fine, then I'm happy with that too.

Copy link
Author

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

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

I cannot say in advance when this will be merged.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

The 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

Copy link
Author

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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:?}"
);
}
}
Loading