Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache some properties #5540

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

Illviljan
Copy link
Contributor

@Illviljan Illviljan commented Jun 27, 2021

Cache some small properties that are rather slow to calculate but doesn't change that often.

Questions that needs to be resolved:

  • Can these properties change during the lifetime of the class? If so the cache needs to be reset when that happens.
  • Related to Should we cache some small properties? #3514
  • Tests added
  • Passes pre-commit run --all-files
  • User visible changes (including notable bug fixes) are documented in whats-new.rst
  • New functions/methods are listed in api.rst

Notes

  • Mixin classes makes it difficult to cache properties. For example ndim in NdimSizeLenMixin cannot be easily be replaced with cache_readonly.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 27, 2021

Unit Test Results

         6 files           6 suites   53m 11s ⏱️
16 200 tests 14 465 ✔️ 1 735 💤 0 ❌
90 396 runs  82 221 ✔️ 8 175 💤 0 ❌

Results for commit 59bd433.

♻️ This comment has been updated with latest results.

@Illviljan Illviljan closed this Jun 27, 2021
@Illviljan Illviljan reopened this Jun 27, 2021
@max-sixty
Copy link
Collaborator

Thanks for kicking this off @Illviljan .

My concern with this is that adding the _cache nullifies the benefits of slots. @keewis do you have any thoughts?

It also sounds like the speed-ups I was seeing might not be real. Probably we should have an ASV so there's a benchmark we're all looking at.

@Illviljan
Copy link
Contributor Author

The case I'm optimizing is dataset interpolation with many variables. Although it's not the bottleneck there I halved the shape time from ~180ms to ~92ms with this change.

@Illviljan Illviljan added the run-benchmark Run the ASV benchmark workflow label Dec 11, 2021
@max-sixty
Copy link
Collaborator

Resurrecting this, as discussed on the dev call.

Could we replace the pandas decorator with the one from the standard library? That may require adding __dict__ as a slot.

Then as long as the benchmarks still look good, there was consensus that we should merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion run-benchmark Run the ASV benchmark workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants