Ensure lon, lat are passed on in Time addition/subtraction #1927
Conversation
astropy/time/core.py
Outdated
There was a problem hiding this comment.
Looking at replicate we probably want to copy 'precision', 'in_subfmt', 'out_subfmt' as well:
# Optional or non-arg attributes
attrs = ('isscalar', '_delta_ut1_utc', '_delta_tdb_tt', # <== These should *not* be copied
'lat', 'lon', 'precision', 'in_subfmt', 'out_subfmt')
Same applies for __add__.
There was a problem hiding this comment.
BTW I thought about trying to somehow use replicate to avoid this hardcoding of the relevant attributes. The problem here is that the _delta_* attributes should not be copied because they get invalidated by the arithmetic. Thus you end up with a net gain of several lines of code, although that solution would be more future-proof to the addition of attributes in the future. Something like this might work:
tai = self_time.replicate()
tai._time.jd1 = jd1
tai._time.jd2 = jd2
tai._time.scale = 'tai'
tai._delta_ut1_utc = None
tai._detla_tdb_tt = None
Anyway, just thinking out loud.
|
@taldcroft - sorry to change the ground, but as it seemed we would need to support a complete location including height anyway, I made another -- hopefully better -- attempt at keeping information around using a new |
|
@mhvk - I've looked at #1928 and it generally looks OK. The only thing is that I think the whole |
|
@taldcroft - you're right, we can at least have this as a bug-fix in 0.3.1, while we work to ensure we have a proper location for 0.4. I updated the PR, following your revised logic about how to implement the arithmetic (and adding extra tests for other properties). |
There was a problem hiding this comment.
Overall this is a very nice simplification of the logic! Just one corner case that I think it isn't handling, that of a length=1 array initializing Time:
In [4]: t1 = Time([1.], format='jd', scale='tai')
In [5]: t1.isscalar
Out[5]: False
In [6]: len(t1._time.jd1)
Out[6]: 1
By your len(jd1) == 1 test below I think this will convert a len-1 array to a scalar. I think you just need a line here with:
result_isscalar = self.isscalar and other.isscalar
So only if both the inputs are scalar then the output is scalar. Then use result_isscalar instead of len(jd1) == 1.
|
Apart from the one comment this looks great. |
|
@taldcroft - yes, indeed, was not quite right. Now use your suggestion (and added tests; I checked the new behaviour is the same as that of numpy). |
|
@taldcroft - think the |
Ensure all relevant Time attributes are passed on in Time addition/subtraction
|
Thanks! |
Ensure all relevant Time attributes are passed on in Time addition/subtraction
Ensure all relevant Time attributes are passed on in Time addition/subtraction
2 participants
Another PR addressing #1924. Now: