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
| # 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?
| 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)
| 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? 😄
| 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.
OracPrime
added
Reviewed
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.