-
Notifications
You must be signed in to change notification settings - Fork 33
ENH: cache helper functions #308
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
array_api_compat/common/_helpers.py:944
- [nitpick] Consider adding a comment to explain why 'cache' is included in _all_ignore to improve clarity for future maintainers.
_all_ignore = ['cache', 'sys', 'math', 'inspect', 'warnings']
array_api_compat/common/_helpers.py
Outdated
from typing import Optional, Union, Any | ||
|
||
from ._typing import Array, Device, Namespace | ||
|
||
|
||
def _is_jax_zero_gradient_array(x: object) -> bool: | ||
@cache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This in theory could lead to a memory leak for a user that somehow dynamically defines and then forgets a lot of classes. I don't think it's something to worry about in real life?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we limit the cache size just not even think about this possibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. @lru_cache(100)
costs just 4ns more than @cache
as long as it doesn't need to evict anything (which in 99% of the times it won't happen). Amended.
9c0ea9c
to
632f081
Compare
dtype = x.dtype # type: ignore[attr-defined] | ||
except AttributeError: | ||
return False | ||
cls = cast(Hashable, type(dtype)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't say I'm happy to see cryptic things from typing
doing something at runtime, but OK, am ready to believe it's somehow useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cast
is a noop at runtime.
Merged, thanks @crusaderky |
Speed up helper functions through caching.
Note:
is_numpy_array
andis_jax_array
are slower than the other equivalent functions due to the reclassification of JAX zero gradient arrays.