London | 25-SDC-Nov | Zohreh Kazemianpour | Sprint 5 | Laptop-allocation #314
Conversation
…dness scores and improve overall allocation quality
zohrehKazemianpour
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.
Some intelligent coding here, nice!
Couple of questions to clear up, but overall good work.
| name: str | ||
| age: int | ||
| # Sorted in order of preference, most preferred is first. | ||
| preferred_operating_system: tuple |
There was a problem hiding this comment.
I used a tuple because Person needs to be hashable to serve as a dictionary key. If I used a list, Python would raise a TypeError: unhashable type: list
There was a problem hiding this comment.
My version of python doesn't complain. What Python version are you using?
There was a problem hiding this comment.
Ah, OK, I now understand it. The data being passed in is Tuples, so when I have preferred_operating_system as a list, Python ignores the type hint and stores the Tuple instead. I'm too used to strongly typed languages!
There was a problem hiding this comment.
Take a gold star. Your Python knowledge exceeds mine in this respect (and probably many others, I'm guessing)
| raise ValueError("Not enough laptops to allocate one per person") | ||
|
|
||
|
|
||
| people_sorted = sorted(people, key = lambda p: len(p.preferred_operating_system)) |
There was a problem hiding this comment.
Nice touch - haven't seen anyone else do this, but it is a clever tweak to the algorithm
There was a problem hiding this comment.
Thank you! I looked into more complex approaches like the Hungarian Algorithm, but found it a bit overwhelming at this stage for me. I decided to stick with greedy approach to ensure the pickest user get a compatible laptop before the more flexible users. With this we can minimise the overall sadness score for the group.
| people_sorted = sorted(people, key = lambda p: len(p.preferred_operating_system)) | ||
|
|
||
| result={} | ||
| available_laptops = laptops.copy() |
There was a problem hiding this comment.
Good spot that you need the copy if you don't want to change the original
| sadness = calculate_sadness(person, laptop) | ||
| if sadness < smallest_sadness: | ||
| smallest_sadness = sadness | ||
| best_laptop = laptop |
There was a problem hiding this comment.
You could of course break out of the loop if sadness is 0
There was a problem hiding this comment.
That is a very good point, thanks for mentioning it. I've added a break when sadness == 0 to avoid unnecessary iterations.
|
|
||
| return result | ||
|
|
||
| allocate_laptops(people, laptops) |
There was a problem hiding this comment.
Why is this first call to allocate laptops here?
There was a problem hiding this comment.
I removed the first call. it was a leftover from debugging. thanks for catching that.
OracPrime
added
Reviewed
Thank you for the detailed review and the kind words! I really appreciate the feedback! |
zohrehKazemianpour
added
the
Needs Review
| name: str | ||
| age: int | ||
| # Sorted in order of preference, most preferred is first. | ||
| preferred_operating_system: tuple |
There was a problem hiding this comment.
My version of python doesn't complain. What Python version are you using?
| name: str | ||
| age: int | ||
| # Sorted in order of preference, most preferred is first. | ||
| preferred_operating_system: tuple |
There was a problem hiding this comment.
Ah, OK, I now understand it. The data being passed in is Tuples, so when I have preferred_operating_system as a list, Python ignores the type hint and stores the Tuple instead. I'm too used to strongly typed languages!
| name: str | ||
| age: int | ||
| # Sorted in order of preference, most preferred is first. | ||
| preferred_operating_system: tuple |
There was a problem hiding this comment.
Take a gold star. Your Python knowledge exceeds mine in this respect (and probably many others, I'm guessing)
2 participants
Learners, PR Template
Self checklist
Changelist
This PR implements laptop allocation.