Repository navigation
PERF: Add cache for some speed-up - #20107
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
|
Is there a need to at least document that there is a cache and if needed, how user can invalidate it? |
|
Lovely! |
|
Constants cannot be changed after you import units, so that should not be an issue. But since a unit's hash is by name of the base, it may well be possible to mess oneself up. If so, the speed-up is perhaps not significant enough to worry about. Let me think a bit about it, and maybe switch steps. In the meantime, I'll convert this to draft. |
|
Given people can define their own units downstream and so on, we should empower them to manage the cache if needed? |
|
I fear we might need to cache by unit |
|
@pllim, @nstarman - I thought a bit more about possible problems with the cache, and really it boils down to whether one can ever get the same hash for two units even though their actual physical units are not the same. As I mentioned above, changing constants is not a possible route: after importing If we agree that is a good route, then the caching here should be safe; I'll rebase once we have reached a decision on #20224. |
Typically, a code does not that many unit conversions, but often does
them multiple times (likely implicitly), so we can speed up things if
we cache the converter:
```
b = np.arange(10.0)
%timeit u.km.to(u.m, b)
on main: 1.24 μs, with cache: 760 ns
q = np.arange(10.0) << u.km
q2 = 1 * u.m
%timeit q + q2
on main: 4.86 μs, with cache: 4.34 μs
q3 = 30*u.deg
%timeit np.sin(q3)
on main: 4.45 μs, with cache: 4.27 μs
```
Note that these improvements are quiet small, in part because I used
relatively simple units for these examples. A more complicated one
(on a different machine):
```
unit = u.Unit("erg/(s.cm2.Hz)"
%timeit unit.to(u.Jy)
on main: 2.5 μs, with cache: 791 ns
```
037bba3 to
c46f63f
Compare
| converter = unit_scale_converter | ||
| else: | ||
| return lambda val: scale * _condition_arg(val) | ||
| converter = lambda val: scale * _condition_arg(val) |
There was a problem hiding this comment.
It would be nice to be able to use the new Scaler here, but _condition_arg prevents it. There's no good way around this, like lowering _condition_arg into C and adding it as an option to Scaler?
There was a problem hiding this comment.
No, it doesn't prevent it: _condition_arg is built into Scaler! This PR is meant as a prelude: once in, I'll rebase #20183 on top. A nice extra is that then the fact that Scaler caches some object construction inside will become useful, since the scaler instance will remain in the converter dict and thus live for a whole session.
Typically, a code does not that many unit conversions, but often does them multiple times (likely implicitly), so we can speed up things if we cache the converter:
Note that these improvements are quiet small, in part because I used relatively simple units for these examples. A more complicated one (on a different machine):
p.s. I have not added a changelog entry since the performance increase for the changes here do not quite warrant that. It will become better with the next steps: to replace the lambda with a C version for further speed improvements for small arrays, and then a special numpy scaled dtype for large arrays.