Use @cache and @cached_property for memoization

[agriyakhetarpal]

Description

Uses @cache and @cached_property via functools to implement memoization. I shall add more commits to this PR for more changes in the remaining files.

Fixes #2288

Type of change

• New feature (non-breaking change which adds functionality)
• Optimization (back-end change that speeds up the code)
• Bug fix (non-breaking change which fixes an issue)

Key checklist:

• No style issues: $ flake8
• All tests pass: $ python run-tests.py --unit
• The documentation builds: $ cd docs and then $ make clean; make html

You can run all three at once, using $ python run-tests.py --quick.

Further checks:

• Code is commented, particularly in hard-to-understand areas
• Tests added that prove fix is effective or that feature works

[agriyakhetarpal]

I assume flake8 seems to be failing because it does not recognise @cache since it was introduced in Python 3.9, though the @cached_property feature existed in Python 3.8 – functools

[valentinsulzer]

I think you have to import cache and cached property from functools. Did this work locally on your machine?

[agriyakhetarpal]

Oh, that makes more sense. I had run python run-tests.py --unit and assumed the problem is with flake8 – (my fault!), though importing functools within the changed files gives

Ran 1193 tests in 3.718s
FAILED (failures=34, errors=811, skipped=59)

I'll push the changes in a moment

[valentinsulzer]

The cached_property methods should not define ._name. They should just return the output of the constructor. i.e.

class DataSet:
    def __init__(self, sequence_of_numbers):
        self._data = sequence_of_numbers

    @property
    @cache
    def stdev(self):
        return statistics.stdev(self._data)

instead of

class DataSet:
    def __init__(self, sequence_of_numbers):
        self._data = sequence_of_numbers

    @property
    @cache
    def stdev(self):
        self._stdev = statistics.stdev(self._data)
        return self._stdev

example is from https://docs.python.org/3/library/functools.html#functools.cached_property

[agriyakhetarpal]

I happened to memoize functions one-by-one and have been able to identify specific functions that cause errors upon memoization (the others are working as necessary):

Ran 1195 tests in 301.155s
FAILED (failures=1, skipped=60)

Would it be recommended to open a separate PR to clear out the mess or should I try and fix individual commits in this one?

P.S. Can this be memoized as well? I do not see any specific test failing upon doing so

[valentinsulzer]

I happened to memoize functions one-by-one and have been able to identify specific functions that cause errors upon memoization (the others are working as necessary):

I see 800 failing tests in this PR

Would it be recommended to open a separate PR to clear out the mess or should I try and fix individual commits in this one?

Keep in same PR

P.S. Can this be memoized as well? I do not see any specific test failing upon doing so

Yes, it can

[agriyakhetarpal]

Apologies, I closed the PR by error (😅) – I removed a commit too many

I'll reopen this with the new changes in a single commit

[codecov]

Codecov Report

Base: 99.72% // Head: 99.72% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (94b7200) compared to base (c8f2890).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2465      +/-   ##
===========================================
- Coverage    99.72%   99.72%   -0.01%     
===========================================
  Files          257      257              
  Lines        19004    18973      -31     
===========================================
- Hits         18951    18920      -31     
  Misses          53       53              

Impacted Files	Coverage Δ
pybamm/expression_tree/symbol.py	100.00% <100.00%> (ø)
...m/models/full_battery_models/base_battery_model.py	99.79% <100.00%> (-0.01%)	⬇️
...s/full_battery_models/lithium_ion/electrode_soh.py	100.00% <100.00%> (ø)
pybamm/simulation.py	100.00% <100.00%> (ø)
pybamm/solvers/solution.py	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[valentinsulzer]

Looks good, try using lru_cache instead of cache for compatibility with python 3.8

[agriyakhetarpal]

I saw that functools also describes using @property and @cache together to implement similar functionality, which can be used in cases wherein using @cached_property failed tests upon implementation; for example in this function variables_and_events

Is this an approach that can be taken forward? This would again not be compatible with Python 3.8 however

[valentinsulzer]

It's not obvious to me why using @cache and @property would work when @cached_property doesn't.
I think variables_and_events is failing for a different reason (probably that the cache needs to be cleared at some point and this isn't happening if using @cached_property).

Happy to merge this as-is if it's ready

[agriyakhetarpal]

Great! I am not aware of any ways (yet) to measure if there is any performance increase by using @cache and @property, I suggested it because no tests failed 😃 I'm happy as it is – I'll push another commit to update the CHANGELOG

Edit: why did these checks fail? The build on 3.9 was passing earlier

[valentinsulzer]

Some of the solvers fail inconsistently, we haven't yet been able to figure out why

[valentinsulzer]

@all-contributors add @agriyakhetarpal for code

[allcontributors]

@tinosulzer

I've put up a pull request to add @agriyakhetarpal! 🎉