Implement exercise saddle-points #400

hekrause · 2017-11-25T21:37:29Z

No description provided.

hekrause · 2017-11-26T11:03:30Z

I don't know why the ensure-readmes-are-updated.sh script has a problem with the README.md.
I generated it with the command ./configlet generate rust --only saddle-points and then pushed it.

petertseng

I don't know why the ensure-readmes-are-updated.sh script has a problem with the README.md.

I will point it out with a line comment.

petertseng · 2017-11-26T11:13:26Z

exercises/saddle-points/README.md

+It has a saddle point at (1, 0).
+
+It's called a "saddle point" because it is greater than or equal to
+every element in its row and the less than or equal to every element in


and the less than

petertseng · 2017-11-26T11:14:25Z

exercises/saddle-points/README.md

+It has a saddle point at (1, 0).
+
+It's called a "saddle point" because it is greater than or equal to
+every element in its row and the less than or equal to every element in


that means you did not take exercism/problem-specifications@2f6a515 into account when generating the README. Please regenerate taking it into account.

coriolinus

Thanks for implementing this exercise, @hekrause! There are a few changes I'd like to see, as described below.

Another change I'd like to see, but which isn't appropriate for a line comment: there are currently no tests which verify that the implementation can detect more than one solution, when more than one exists. Please add at least the case in the canonical data to ensure that implementations are complete.

coriolinus · 2017-11-26T15:36:05Z

exercises/saddle-points/Cargo.toml

@@ -0,0 +1,5 @@
+[package]
+name = "saddle-points"
+version = "1.1.0"


This version should match the version in canonical-data.json. That means that as of the current master branch of problem-specifications, it should be 1.0.0.

coriolinus · 2017-11-26T15:41:23Z

exercises/saddle-points/src/lib.rs

@@ -0,0 +1,4 @@
+
+pub fn find_saddle_points(input: Vec<Vec<u64>>) -> Vec<(u64, u64)>{


There's no need to transfer ownership of the input for find_saddle_points to work. We want to teach people how to write idiomatic Rust, so in this case, this function should take &[&[u64]] (reference to slice of reference to slice of u64). Vec<Vec<u64>> should coerce transparently to the slice form.

coriolinus · 2017-11-26T15:42:37Z

exercises/saddle-points/src/lib.rs

@@ -0,0 +1,4 @@
+
+pub fn find_saddle_points(input: Vec<Vec<u64>>) -> Vec<(u64, u64)>{


Naming the variable input causes a compiler warning due to the unused variable. While this doesn't break CI, I'd prefer to skip that by renaming the variable to _input or simply _.

coriolinus · 2017-11-26T15:43:23Z

exercises/saddle-points/tests/saddle-points.rs

+use saddle_points::*;
+
+#[test]
+fn identify_single_saddle_point() {


All test names should begin with test_.

coriolinus · 2017-11-26T15:49:32Z

exercises/saddle-points/tests/saddle-points.rs

+
+#[test]
+#[ignore]
+fn identify_missing_saddle_point() {


Use of "missing" in this test name implies that one should be present. Please update the name of this function to make it clear that there are expected to be none.

I believe this is based on the canonical test "Can identify lack of saddle points when there are none". If so, consider naming this test based on that name. The test names don't need to precisely map to the canonical test descriptions, but it's best when there is at least some correspondence.

coriolinus · 2017-11-26T15:50:54Z

exercises/saddle-points/tests/saddle-points.rs

+#[ignore]
+fn identify_missing_saddle_point() {
+    let input: Vec<Vec<u64>> = vec![vec![1, 2, 3],
+                                    vec![3, 2, 1],


If this test is based on the canonical test "Can identify lack of saddle points when there are none", this row of input data diverges from the equivalent row in the canonical data. Is there a reason for this?

No, it was a typo. :-)

coriolinus · 2017-11-26T15:55:29Z

exercises/saddle-points/tests/saddle-points.rs

+
+#[test]
+#[ignore]
+fn non_quadratic_matrix_high() {


Non-imperative quibble: change *quadratic* test names to use the term square instead. While "quadratic" isn't wrong, it has associations with the operation of squaring more than the shape of a matrix. Also, it's usually better to name things for clarity, and "square" is a much more common word.

If you feel strongly that the term should remain, it's fine; I don't feel that this is an essential change, just one I'd prefer.

petertseng · 2017-11-26T22:35:27Z

exercises/saddle-points/example.rs

+pub fn find_saddle_points(input: Vec<Vec<u64>>) -> Vec<(u64, u64)>{
+    let mut saddle_points: Vec<(u64, u64)> = Vec::new();
+
+    let row_index = input.len();


Although this is indeed an index, it is also not the index of any existing element in the input (it is the index of the first element beyond it), so to avoid confusion I would call this height instead.

petertseng · 2017-11-26T22:35:39Z

exercises/saddle-points/example.rs

+    let mut saddle_points: Vec<(u64, u64)> = Vec::new();
+
+    let row_index = input.len();
+    let column_index = input[0].len();


Although this is indeed an index, it is also not the index of any existing element in the input (it is the index of the first element beyond it), so to avoid confusion I would call this height instead.

Same, except now width.

petertseng · 2017-11-26T22:36:58Z

exercises/saddle-points/example.rs

+
+    for i in 0..row_index {
+        for j in 0..column_index {
+            let row = &input[i];


since this is only based on i and not j, it would be possible to assign it in the outer loop and not the inner one. Unless that makes the code less understandable.

petertseng · 2017-11-26T22:37:10Z

exercises/saddle-points/example.rs

+            let row = &input[i];
+            let column = input.iter().map(|x| x[j]).collect::<Vec<u64>>();
+
+            let max = row.iter().max().unwrap();


since this is only based on row, which is only based on i and not j, it would be possible to assign it in the outer loop and not the inner one. Unless that makes the code less understandable.

If the matrix is empty the two for loops prevent an unwrap on row in line 13.

petertseng · 2017-11-26T22:40:42Z

exercises/saddle-points/example.rs

+            }
+        }
+    }
+    return saddle_points;


I suggest removing the return and the semicolon.

However, I cannot find a justification for this suggestion in https://aturon.github.io nor https://github.com/rust-lang-nursery/fmt-rfcs/blob/master/guide/guide.md which means I have no grounds backing up this suggestion. Nevertheless, it is suggested.

coriolinus · 2017-11-29T12:07:22Z

Thanks @hekrause for updating these. As mentioned before (#400 (review)), there still isn't a test which validates that solutions can return multiple saddle points if they exist; there's a case for that already in the canonical data. Please add at least that case.

coriolinus · 2017-11-29T12:12:11Z

exercises/saddle-points/tests/saddle-points.rs

-                                    vec![5, 3, 2],
-                                    vec![6, 6, 7]];
+fn test_identify_single_saddle_point() {
+    let input: &[&[u64]] = &[&[9, 8, 7],


Interesting; I hadn't intended that you change the tests when I asked you to change the signature; a reference to Vec<Vec<u64>> should just work for a function which requires &[&[u64]].

I'm not sure whether to ask you to change this back. As of right now, I'm undecided: on the one hand, it's interesting to see that slice reference literals work at all. On the other hand, idiomatic code would be to specify the inputs as Vec<Vec<u64>> and then pass the reference and let the Deref trait do the work for you.

I'd like to solicit opinions from the other maintainers about which of these forms are better style.

Since it is idiomatic, I would keep the vec then. Will be even more convinced if finding a source that tells us this, but I cannot.

We do have quite a few instances of using slices in tests in other exercises, but potentially changing them is a story for another day

So you mean in the tests should be vectors as in the first version but I should call the function with the &[&[u64]] parameters?And the slices should be from the vectors?

Yes: leave the function signature as it is, which expects slices of slices, and change the tests so that they allocate Vecs and pass a reference to the function.

petertseng · 2017-11-30T08:27:41Z

exercises/saddle-points/example.rs

+
+            let column = input.iter().map(|x| x[j]).collect::<Vec<u64>>();
+            let row = &input[i];
+            let max = row.iter().max().unwrap();


since this is only based on row, which is only based on i and not j, it would be possible to assign it in the outer loop and not the inner one. Unless that makes the code less understandable.

If the matrix is empty the two for loops prevent an unwrap on row in line 13.

Indeed, if a row is empty. In that case, I will now only optionally ask for row to be moved to the outer loop (it still can be), and agree that I was wrong to ask for this max line to be moved; it cannot be.

Yes you are right but I would like to leave it like this because I think in the way it is now it is more readable.

petertseng · 2017-11-30T08:29:53Z

exercises/saddle-points/src/lib.rs

@@ -0,0 +1,4 @@
+


this empty line could be removed, and I think it should be. The fact that there aren't any use lines shouldn't affect things I don't think, because if they were present, we wouldn't start the file with an empty line either.

same

petertseng · 2017-11-30T08:29:56Z

exercises/saddle-points/example.rs

@@ -0,0 +1,24 @@
+


this empty line could be removed, and I think it should be. The fact that there aren't any use lines shouldn't affect things I don't think, because if they were present, we wouldn't start the file with an empty line either.

The empty line I intended to be removed has been removed. GitHub is now placing this comment on an unrelated line, so let us ignore it.

coriolinus · 2017-12-01T10:08:12Z

exercises/saddle-points/tests/saddle-points.rs

+    let vector: Vec<Vec<u64>> = vec![vec![9, 8, 7],
+                                     vec![5, 3, 2],
+                                     vec![6, 6, 7]];
+    let input: &[&[u64]] = &[&vector[0][..],


This way of forming the input slice of slices is ugly, and is my fault. I inadvertently gave you bad advice. The find_saddle_points signature should actually be pub fn find_saddle_points(_: &[Vec<u64>]) -> Vec<(u64, u64)>.

I was wrong to tell you that it should accept an &[&[u64]], because the compiler doesn't automatically dereference the inner elements into a slice; the inner elements need to remain Vec<u64>. At the same time, the outer layer should be &[_] because it is idiomatic to pass a slice reference, not to transfer ownership of a Vec.

In other words, the function signature and tests should work as in this playground example.

I apologize for the bad advice. Now please change the signature one more time to hopefully clean this up once and for all.

Probably the best function signature for real projects would look like this:

pub fn find_saddle_points<X, Y, T>(_: &Y) -> Vec<(usize, usize)> where Y: AsRef<[X]>, X: AsRef<[T]>, T: PartialOrd, { unimplemented!() }

However, that would be an intimidating signature to require of new students. I still prefer &[Vec<u64>].

coriolinus · 2017-12-01T15:03:03Z

exercises/saddle-points/example.rs

+            let value = input[i][j];
+
+            if value >= *max && value <= *min {
+                saddle_points.push((i as u64, j as u64));


In fact, we could eliminate the casts here by having the function return Vec<(usize, usize)>. I prefer that over the current version.

I asked for some updates to the signature two weeks ago, but there's been no movement. In the interest of getting this exercise into a mergeable state, I went ahead and performed the requested updates myself: - exercism#400 (review) - exercism#400 (review)

coriolinus

As it's been two weeks, I've gone ahead and made the changes I requested myself. I'm now happy with the state of this exercise.

hekrause and others added 17 commits October 17, 2017 18:51

Added tests an more README info.

e04e3c0

Duplicate test let CI fail.

cd20bcf

Don't know why warnings are only displayed on CI.

d9c2e9d

Deny warnings on wrong place.

6998c3e

Removed tests which failed only on CI.

1441317

Merge branch 'master' into master

168135d

Remove changes.

fce8915

Merge branch 'master' of https://github.com/hekrause/rust

3833a54

pascals-triangle: 1.2.0

20c0ab0

pascals-triangle: move four between three and five

834b743

Merge branch 'master' of https://github.com/exercism/rust

50b0d69

Remove duplicated tests.

72b1604

Remove diffie-hellman

c784f9d

Initial saddle-points commit.

1d04e9a

Remove diffie-hellman left-overs.

f2c139d

Remove old README.md

c5e0144

Generate new README.md

c6fa7ff

petertseng reviewed Nov 26, 2017

View reviewed changes

Try only to change README.md

bb710fc

coriolinus suggested changes Nov 26, 2017

View reviewed changes

petertseng reviewed Nov 26, 2017

View reviewed changes

Made requested changes.

fb0f63b

coriolinus reviewed Nov 29, 2017

View reviewed changes

petertseng reviewed Nov 30, 2017

View reviewed changes

Change tests again.

45c002d

coriolinus reviewed Dec 1, 2017

View reviewed changes

Fix merge conflict with master

b64d368

coriolinus and others added 3 commits December 14, 2017 13:01

Merge branch 'master' into saddle-points

1f07992

Rename test *quadratic* -> *square*

badcc30

coriolinus approved these changes Dec 14, 2017

View reviewed changes

coriolinus merged commit 9a2b8d9 into exercism:master Dec 14, 2017

petertseng mentioned this pull request Mar 10, 2018

Use &[T] instead of &Vec<T> where possible #454

Merged

		@@ -0,0 +1,4 @@

		pub fn find_saddle_points(input: Vec<Vec<u64>>) -> Vec<(u64, u64)>{

Implement exercise saddle-points #400

Implement exercise saddle-points #400

Uh oh!

Conversation

hekrause commented Nov 25, 2017

Uh oh!

hekrause commented Nov 26, 2017

Uh oh!

petertseng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coriolinus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coriolinus commented Nov 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hekrause Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petertseng Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coriolinus Dec 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coriolinus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hekrause Nov 30, 2017 •

edited

Loading

petertseng Nov 30, 2017 •

edited

Loading

coriolinus Dec 1, 2017 •

edited

Loading