WestMidlands| SDC-NOV-2025| Sara Tahir| Sprint 5| Feature/laptop allocation #288
Conversation
|
Your PR's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Sprint part (SPRINT 5) doesn't match expected format (example: 'Sprint 2', without quotes) If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
SaraTahir28
changed the title
SaraTahir28
added
the
Needs Review
OracPrime
added
the
Review in progress
OracPrime
left a comment
OracPrime
left a comment
There was a problem hiding this comment.
I've been picky in the comments here, because if I wasn't being picky I wouldn't have much to say - it's solid code apart from the one type bug on the return value
laptop Allocation/laptop.py
Outdated
| # Map people to laptops | ||
| allocation = {} | ||
| for i, j in zip(row_indices, col_indices):#it pairs up results (person indices and laptop indices). The loop then uses those pairs to build the final allocation dictionary | ||
| allocation[people[i].name] = laptops[j] |
There was a problem hiding this comment.
You've declared the function as Dict[Person,Laptop] but are return Dict[str,Laptop]. I'm guessing you actually want the former?
There was a problem hiding this comment.
Yes I did and I fixed it.The discussion around the os behaving as a tuple in our saturday session was helpful here for me
laptop Allocation/laptop.py
Outdated
| if laptop.operating_system == person.preferred_operating_systems[0]: | ||
| return 0 | ||
| elif len(person.preferred_operating_systems) > 1 and laptop.operating_system == person.preferred_operating_systems[1]: | ||
| return 1 |
There was a problem hiding this comment.
Hard-coding the 0,1,2 feels clunky here. If you're explicitly saying that if it's not in the top 3 it's terrible, then that's one thing. But this code looks like a failure to use index to get the ranking of the laptop. Either explicitly have a "maximum rank for any happiness" (as you appear to have here, but don't say) or use and index (making sure it copes with not found correctly)
There was a problem hiding this comment.
I have Updated the sadness function , its no longer hard‑coded. The score is now calculated using the index of the laptop’s operating system within the person’s preference tuple. Anything outside the top three ranks is treated as a poor match and returns the TERRIBLE_SADNESS .
| Laptop(id=3, manufacturer="Dell", model="XPS", screen_size_in_inches=15, operating_system=OperatingSystem.UBUNTU), | ||
| Laptop(id=4, manufacturer="Apple", model="macBook", screen_size_in_inches=13, operating_system=OperatingSystem.MACOS), | ||
| Laptop(id=5, manufacturer="Apple", model="macBook", screen_size_in_inches=13, operating_system=OperatingSystem.MACOS), | ||
| Laptop(id=6, manufacturer="Dell", model="macBook", screen_size_in_inches=13, operating_system=OperatingSystem.ARCH), |
There was a problem hiding this comment.
Dell have started making macBooks? who knew? 😄
There was a problem hiding this comment.
well One day they might have a merger :O
| for j, laptop in enumerate(laptops): | ||
| sadness_matrix[i,j] = sadness(person, laptop) #this matrix represents complete laptop allocation | ||
| # Hungarian algorithm to run on our sadness matrix | ||
| row_indices, col_indices = linear_sum_assignment(sadness_matrix) #which person which laptop |
There was a problem hiding this comment.
You're using a "black box" algorithm here. It is entirely opaque what it is doing or how it is working. For me, this would trigger adding some test files to check that it is working how I expect it to.
There was a problem hiding this comment.
very good idea.I have added one very basic test to check this.
There was a problem hiding this comment.
Except that with one person and one laptop, it's hardly testing the algorithm.
OracPrime
added
Reviewed
SaraTahir28
added
the
Needs Review
OracPrime
left a comment
OracPrime
left a comment
There was a problem hiding this comment.
Because the rest of the code is so good, I think I'm going to push back on the test. It's a bit too trivial.
| for j, laptop in enumerate(laptops): | ||
| sadness_matrix[i,j] = sadness(person, laptop) #this matrix represents complete laptop allocation | ||
| # Hungarian algorithm to run on our sadness matrix | ||
| row_indices, col_indices = linear_sum_assignment(sadness_matrix) #which person which laptop |
There was a problem hiding this comment.
Except that with one person and one laptop, it's hardly testing the algorithm.
OracPrime
removed
the
Needs Review
|
well that makes sense. I have added more tests to test different allocations. Thanks. |
SaraTahir28
added
the
Needs Review
OracPrime
left a comment
OracPrime
left a comment
There was a problem hiding this comment.
I'm going to flag this as complete, because we're reaching diminishing returns. I'd still be happier with a test with more than one person and one laptop, but I'm not going to insist on it.
OracPrime
added
Complete
2 participants
Learners, PR Template
Self checklist
Changelist
Used Hungarian Algorithm to minimize sadness score across all lptop users.
Questions
Kindly let me know which approah was better to solve this issue for the purpose of this assignmnent ? Thanks.