Skip to content

Comments

Sheffield | 25-SDC-Nov | Sheida Shabankari | Sprint 2 |improve with caches#112

Open
sheida-shab wants to merge 3 commits intoCodeYourFuture:mainfrom
sheida-shab:Feat-improve-with-caches
Open

Sheffield | 25-SDC-Nov | Sheida Shabankari | Sprint 2 |improve with caches#112
sheida-shab wants to merge 3 commits intoCodeYourFuture:mainfrom
sheida-shab:Feat-improve-with-caches

Conversation

@sheida-shab
Copy link

  • I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • My changes meet the requirements of the task
  • I have tested my changes
  • My changes follow the style guide

PR summary :

Add manual caching to speed up recursive Fibonacci and ways_to_make_change functions.

  • Use dictionaries to store results of already computed subproblems.
  • In Fibonacci, store fibonacci(n) values to avoid recalculating them.
  • In ways_to_make_change, use a key (total, tuple(coins)) to remember results for each combination of total and remaining coins.
  • All recursive calls share the same cache to reuse previous results, without using @cache.
  • These changes reduce repeated calculations and make the functions much faster for large inputs.

@sheida-shab sheida-shab added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Module-Complexity The name of the module. labels Feb 17, 2026


def ways_to_make_change_helper(total: int, coins: List[int]) -> int:
def ways_to_make_change_helper(total: int, coins: List[int],cache=None) -> int:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use {} as the default value for cache?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I use {} as the default value, the cache is shared between all function calls .
Not only between recursive calls, but between every call to the function in the whole program.

This happens because default arguments in Python are created once, when the function is defined.

If I use None instead, then every time ways_to_make_change is called,
a new cache dictionary is created.

At the same time, all recursive calls share that same cache,
which is exactly what we want for memoisation .

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarification. I didn't know that.

Here are two alternatives,

  1. Create the cache in ways_to_make_change() and then pass it to ways_to_make_change_helper(), or

  2. Define ways_to_make_change_helper() as an inner function of ways_to_make_change(), and then have
    the inner function share the cache declared in ways_to_make_change().

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestions!

I’ve updated the code again based on your guidance. I now create the cache in
ways_to_make_change() and pass it into the helper function, so the cache is scoped
to a single top-level call but shared across all recursive calls.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Array creation is a relatively costly operation.

From line 34, we know coins can only be one of the following 9 arrays:

[200, 100, 50, 20, 10, 5, 2, 1]
[100, 50, 20, 10, 5, 2, 1]
[50, 20, 10, 5, 2, 1]
...
[1]

We could further improve the performance if we can

  • avoid repeatedly creating the same sub-arrays at line 41 (e.g. use another cache), and
  • create key as (total, a_unique_integer_identifying_the_subarray) instead of as (total, tuple of coins)
    • There are only a small number of different subarrays. We can easily assign each subarray a unique integer.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor memoisation to avoid repeated sub-array creation

Precompute all possible coin sub-arrays once and identify them by index.
Use (total, subarray_id) as the cache key instead of (total, tuple(coins)),
so recursive calls reuse the same sub-arrays and avoid unnecessary list
and tuple allocations.

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Feb 22, 2026
@sheida-shab sheida-shab added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Feb 24, 2026
@cjyuan
Copy link

cjyuan commented Feb 24, 2026

Looks good. Well done.

@cjyuan cjyuan added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Feb 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Complete Volunteer to add when work is complete and all review comments have been addressed. Module-Complexity The name of the module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants