London | 25-SDC-July | Eyuel Abraham | Sprint 2 | Improve with caches #149
London | 25-SDC-July | Eyuel Abraham | Sprint 2 | Improve with caches #149eyuell21 wants to merge 4 commits intoCodeYourFuture:mainfrom
Conversation
eyuell21
added
the
Needs Review
This comment has been minimized.
This comment has been minimized.
github-actions
bot
removed
the
Needs Review
This comment has been minimized.
This comment has been minimized.
eyuell21
added
the
Needs Review
SlideGauge
added
the
Review in progress
SlideGauge
left a comment
SlideGauge
left a comment
There was a problem hiding this comment.
Overall decent approaches with solving via caching/memoization. I've got a couple of requests, which will polish both solutions, could you fulfil them?
| else: | ||
| intermediate = ways_to_make_change_helper(total - total_from_coins, coins=coins[coin_index+1:]) | ||
| ways += intermediate | ||
| ways += ways_to_make_change_helper(remainder, coins[coin_index + 1:], memo) |
There was a problem hiding this comment.
Overall good approach with memoization, but I have one tiny request here:
What happens when we do slicing in Python? (i.e. do varOfTypeList[x:y])? Does it allocate additional memory, potentially a lot of? Can we avoid additional allocation of a lot of memory?
There was a problem hiding this comment.
In Python, doing list[x:y] creates a new list containing the elements from index x to y-1. This requires allocating additional memory proportional to the slice length. we can use a module called intertools (itertools.islice), which returns a lazy iterator over that range instead of creating a new list to avoid excess memory allocation.
SlideGauge
added
Reviewed
|
Hello @SlideGauge, I've made the requested changes. |
|
Hello! @illicitonion Can I get a review on this one please? Thank you! |
illicitonion
left a comment
illicitonion
left a comment
There was a problem hiding this comment.
Generally looks good - one documentation question
|
|
||
|
|
||
| def ways_to_make_change_helper(total: int, coins: List[int]) -> int: | ||
| def ways_to_make_change_helper(total: int, coins: List[int], memo: Dict[Tuple[int, int], int]) -> int: |
There was a problem hiding this comment.
I'd probably document what the cache key here is in the docstring - it's not obvious what the two values in the tuple are (and it took me a minute to work out why len(coins) was a valid thing to include in the cache key at all).
illicitonion
added
Complete
3 participants
Learners, PR Template
Self checklist
Changelist
Added memoization to improve performance.
Questions
No Questions.