Gale-Shapley implementation in Rust #715

lazyprop · 2020-06-12T20:06:10Z

This is, more or less, a translation of the Python implementation.

Amaras · 2020-06-12T20:57:33Z

Hello, thanks for your first PR here! As I'm just a contributor here, and I've only just started learning Rust, I cannot really review your code. That said, it does seem quite clean. I have not tried to build it, but I trust you on that for now before any real review is done.

Problem is, there is an open PR (#687) on my part to improve the Python implementation, so you might want to branch off from simply translating the current Python implementation, and maybe write more idiomatic Rust if possible.
The review in that PR might also give you some ideas to improve your code from the direct translation it seems to be.

In any case, thanks again for your interest in the AAA!

lazyprop · 2020-06-13T04:26:07Z

I have already kept in mind the changes that you have made in the Python implementation.

No 'getters/setters' are used. There is just an initperson function which I think is necessary because initializing a person requires a lot of repetitive lines.
Currently, the number and preferences of people hardcoded because a variable number of people would require randomly generating preferences. The problem is that rand is an external crate in Rust which has to be downloaded from the Internet.
I am new to Rust myself. Nevertheless, I have tried my best to write idiomatic Rust.
I have updated println!("Pairs: ") to println!("Stable Matching: ") to make the output more descriptive.
I have added my name to CONTRIBUTORS.md

Liikt

All in all I am not happy with the way you link men to the women. The way of indexing is rather complex and hard to parse mentally.

Also I would like it if you could use more iterator logic. This would make the code better readable and a bit more concise.

Liikt · 2020-06-13T23:29:02Z

contents/stable_marriage_problem/code/rust/stable_marriage.rs

+#[derive(Eq, PartialEq, PartialOrd)]
+struct Person {
+    name: char,     // a single character denoting the person e.g 'A', 'B', 'F'
+    index: usize,   // index in men/women vecotr


There is typo here. It should be vector.

Liikt · 2020-06-13T23:29:21Z

contents/stable_marriage_problem/code/rust/stable_marriage.rs

+
+impl Person {
+    fn propose_to_next(&mut self, women: &mut Vec<Person>) {
+        /* propose to next most preferred person */


I would use // for normal comments and /// for doc strings.

Liikt · 2020-06-13T23:30:49Z

contents/stable_marriage_problem/code/rust/stable_marriage.rs

+        /* propose to next most preferred person */
+
+        if self.pref_index >= self.preference.len() {
+            ()


Use return here as it is way cleaner than ().

Liikt · 2020-06-13T23:32:39Z

contents/stable_marriage_problem/code/rust/stable_marriage.rs

@@ -0,0 +1,136 @@
+#[derive(Eq, PartialEq, PartialOrd)]


All these are not needed.

Liikt · 2020-06-13T23:46:24Z

contents/stable_marriage_problem/code/rust/stable_marriage.rs

+
+    /* Resolve */
+    let mut cont = true;
+    while cont {


use a named break here:

... 'resolve: loop { ... for man in &men { if man.partner.is_none() { break 'resolve; } } } ...

Liikt · 2020-06-14T00:48:06Z

contents/stable_marriage_problem/code/rust/stable_marriage.rs

+        }
+    }
+
+    println!("\nStable Matching: ");


This is only personal preference but i would rather write

print!("\n"); print!("Stable Matching: \n");

than println!("\nStable Matching: ");

I think rust recommends to use just a println!() invocation without parameters. I think clippy even has a lint for that.

so:

println!(); println!("Stable Matching: ");

Liikt · 2020-06-14T00:48:32Z

contents/stable_marriage_problem/code/rust/stable_marriage.rs

+
+    println!("\nStable Matching: ");
+    for man in &men {
+        match man.partner {


You can do a simple unwrap here as all man must have a partner at this point.

lazyprop · 2020-06-14T15:03:30Z

@Liikt I have rewritten the entire implementation. It is much more concise and 'Rusty' now. The initial declaration and display of the vectors takes up a lot of lines though.

fanninpm · 2020-06-17T23:48:55Z

contents/stable_marriage_problem/code/rust/stable_marriage.rs

+    for man in men {
+        let partner_id = stable_matching.get(&man.id);
+        match partner_id {
+            Some(p) => println!("{} + {}", man.id, p),
+            None => ()
+        }
+    }


I get the feeling that iter and .filter_map() might be useful here. (I'm not sure, though.)

@fanninpm How would we use .filter_map() here? Won't it just over complicate things? We will have to iterate and then collect into vector and then print the vector. I am not sure though. Is there a better way to do this?

@n1trob4ct3r You can use .for_each() on the iterator, but again, I doubt its usefulness. Besides, you refer to both man.id and partner_id in your solution.

@fanninpm Yes that would be a problem too.

Is there anything else? Is it ready for merging?

jdonszelmann · 2020-07-05T09:21:54Z

contents/stable_marriage_problem/code/rust/stable_marriage.rs

+#[derive(Hash, Eq, PartialEq, Copy, Clone)]
+struct PersonId(pub char);
+
+impl fmt::Display for PersonId {


Maybe you want to just derive debug, instead of manually implementing display, to simplify the entire code a bit. Even if you don't want to remove the Display implementation, deriving debug is never a bad practice

I now see that your code relies on the exact representation quite a bit, so then maybe just add a derive debug?

Amaras

So... reviewing the details is good, but the algorithm implemented does not necessarily produce a stable pairing. Even though we discussed it on Discord previously, I think it needs to be here.
What it does is assign each man to its first free preference, in a first come first served way. It does not take into account the preferences of the women, which is why the pairing is not stable in general.

Your previous implementation was demonstrably more correct, so I would approve getting back to that state.

Gale-Shapley implementation in Rust

868529c

improved readability. added name to CONTRIBUTORS.md

ea35907

Liikt requested changes Jun 14, 2020

View reviewed changes

rewritten implementation

09e3578

lazyprop requested a review from Liikt June 14, 2020 14:56

fanninpm reviewed Jun 17, 2020

View reviewed changes

berquist added the Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.) label Jul 4, 2020

jdonszelmann approved these changes Jul 5, 2020

View reviewed changes

Amaras mentioned this pull request Jul 5, 2020

WIP: Further improved stable marriage in python #726

Closed

Amaras requested changes Jul 5, 2020

View reviewed changes

lazyprop closed this Oct 2, 2020

Uh oh!

Gale-Shapley implementation in Rust #715

Gale-Shapley implementation in Rust #715

Uh oh!

Conversation

lazyprop commented Jun 12, 2020

Uh oh!

Amaras commented Jun 12, 2020

Uh oh!

lazyprop commented Jun 13, 2020

Uh oh!

Liikt 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!

lazyprop commented Jun 14, 2020

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!

Amaras left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!