Repository navigation
Improve efficiency of getattr(tm, scale), particularly access to jd1/jd2 #4309
Description
Activity
From @eteq:
I like @taldcroft's suggestion here. The first thing I thought was "I should be able to do
time.tt.jd1, time.tt.jd2, because that's the clearest. But then I checked and realized it's not cached, so I decided it wasn't fast enough given that this is in inner loops. So I naturally assumed there would be a cache and was surprised that there wasn't.If it helps any, there's precedence in coordinates itself for this. So I'd advocate for instead doing just as @taldcroft says here.
I think that's a "new feature", though, so it probably has to wait for 1.2 . In the meantime, I will create a utility function as @mhvk suggests just for coordinates, which we can later change to just being
return getattr(time, scale).jd1, getattr(time, scale).jd2or just replace all of it with direct conversions once the caching and such is in.The implementation of caching is not very complex, so the real question is whether it introduces the possibility for user error with hard-to-debug or inobvious problems.
So in what ways could a cached transform become stale? The obvious issue is in the
delta_ut1_utcanddelta_tdb_ttattributes. If those change then transform values can change. Each of those are properties so there is no problem in clearing the cache if they are set liketm.delta_ut1_utc = ... The vulnerability is updating values in place. It seems like we could set thewritableflag to False, though I don't have much experience with that in practice.Otherwise one workaround is a new property like
tm.jd1_jd2which returns a tuple of(tm.jd1, tm.jd2).I will say that I really like the idea of caching if we can convince ourselves that it is robust. It prevents code like below where a new variable is created just to avoid re-doing the transform.
tm = Time(.., scale='utc') tm_ut1 = tm.ut1 ...Apart from robustness, one possible issue with caching is memory usage; suppose one has just generated a very long UTC time array, and want to convert to TDB. If one writes it like
times = Time(np.arange(...), scale='utc').tdbwill one now automatically have two copies of that very large array around?
That said, I see the benefit of not having explicitly keep the various scales around oneself... And one could in principle have a maximum size up to which one caches.
That said, I see the benefit of not having explicitly keep the various scales around oneself... And one could in principle have a maximum size up to which one caches.
I like this solution - it should probably be a configuration variable given that it's rather machine-specific, but I think this is the least of the various possible evils. It could presumably also be set to
0to turn off the caching if desired.Another useful thing to do might be to have a
clear_cachepublic method that you can use to clean it out. That way, for those who really care about the performance implications, they can clear when needed, but the 98% of cases where it's not too serious can just leave it be.I had previously thought about the question of whether the cache dict would be copied (shallow copy) when replicating and decided that might be a bad idea. So in your example the original UTC
Timeref would be dropped. Conversely, thetimesobject would NOT have the UTC version initially in its cache.I think that in most normal use cases the memory footprint wouldn't be expanded at all.
[EDIT: I updated the code block below a few times...]
# A pattern where both UTC and TT times are needed in local processing tm = Time(.., scale='utc') # old way tm_tt = tm.tt tt_jd1, tt_jd2 = tm_tt.jd1, tm_tt.jd2 # new way tt_jd1, tt_jd2 = tm.tt.jd1, tm.tt.jd2 # Another pattern where the input is UTC but TT is needed for working tm = Time(some_utc_times_from_a_file, scale='utc') tm = tm.ttIn both cases there would be no difference in memory.
@taldcroft - OK, that removes one worry. [I had thought that the TDB item being in the UTCs cache would keep it alive, but that was backwards: the UTC one would not have references anymore, so would get dropped, and that removes one reference to the TDB one, but that stays alive because it is has another reference.]
And we could indeed have a public
clear_cachefunction. Indeed, since one needs it anyway for use indelta_ut1_utcanddelta_tdb_tt, it is mostly a question of whether one would want to expose it. Or maybe more simple: expose thecacheitself, so that one would dodel times.cacheif so inclined (cachecould be a property, so that it has a deleter associated with it).Yes, I like
del times.cacheinstead of aclear_cachemethod.Sounds like we're converging. Will you turn your example in a PR? (I can also do it, but probably some time late next week.)
And exposing the cache as public gives the power users visibility into what's happening.
Good. I probably won't work on it for a bit either, so let's just leave it open to whoever gets there first (but put a note here when starting).
Yes, I like del times.cache instead of a clear_cache method.
👍 as long as it's specifically documented somewhere that this is the trick
Yes, I imagine a specific section on
Cachingor evenPerformance and caching. This would explain how it works and highlight the usage patterns that we just discussed.
When accessing an arbitrary time scale for a
Timeobject, the internal object is always replicated. This came up in #4302.What about:
selfifattr == self.scale?jd1andjd2after transforming will be efficient?Roughly: