Refactor `Time` and `TimeDelta` to inherit from common `TimeBase` class by taldcroft · Pull Request #10656 · astropy/astropy

taldcroft · 2020-08-14T13:17:35Z

Description

This pull request is to address a longstanding issue that TimeDelta inherited from Time but many of the methods (e.g. light_travel_time() were really not applicable. Instead now there is a base class TimeBase that has the common methods and each independently inherits from TimeBase.

After this refactor there were only two problems that showed up in the entire astropy code base, both related to assuming Time as the base class. In one case TimeBase.insert() this was fixed using self.__class__ and in the other Table.to_pandas() using TimeBase. There were no other code changes apart from moving methods around intact.

Fixes #6828
Fixes #10654.

taldcroft · 2020-08-14T14:23:37Z

@mhvk - check out e8af382, which (I think) solves the whole subclass problem and would fix #10654.

I don't have time right now to write the tests, but basically I got expected results with:

t = Time.now()
class MyTime(Time):
    pass
mt = MyTime.now()

MyTime(t, format='iso')
Time(mt, format='iso')

Note that in new and init one now has hardwired references to Time or TimeDelta, which are the prerequisite base classes in order to work properly.

I can put this change in a separate PR if you prefer.

taldcroft · 2020-08-14T14:24:42Z

Also note that last commit is a bit of a WIP, there are docstring updates and other possible issues which I'll fix.

taldcroft · 2020-08-14T15:03:09Z

This is failing on one TimeSeries test, will check.

mhvk

@taldcroft - this is fantastic! And I think some kudos are due for writing things such that this refactor causes so little problems!!

In-line a comment about SCALES and FORMATS which should also be moved. Similarly, _set_scale, __add__ and __sub__ should move to Time - heck, it is probably easier to just make a quick PR to your PR.... forthcoming!

mhvk · 2020-08-14T21:17:55Z

            raise TypeError("Cannot specify both 'time' and 'time_start'")

-        if time is not None and not isinstance(time, Time):
+        if time is not None and not isinstance(time, (Time, TimeDelta)):


Would it make sense to just import TimeBase and check for that?

I thought about this, but in the end I think that the code is more readable and explicit this way, as in it tells the non-expert reader immediately that the results apply to both absolute and delta times.

It's an arbitrary line, but I thought that code inside of the time subpackage should use TimeBase (internal) while the rest would be explicit. I guess there is some philosophy here -- if we or someone else adds a new subclass of TimeBase, do we want that to be in or out from consideration? One can make valid arguments both ways.

@taldcroft - indeed, and the advantage of Time, TimeDelta was that both were imported already. Note that I changed it to TimeBase in the later commit, but that was really because I thought that might be the reason for the doctest failure (which was instead that the same change had to be made in binned.py).

pep8speaks · 2020-08-14T22:41:41Z

Hello @taldcroft 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2020-10-03 10:38:06 UTC

mhvk · 2020-08-14T22:49:16Z

@taldcroft - I pushed directly to your branch some changes (in separate commits, just in case you'd prefer to do things differently); I also amended your TimeSeries commit to do isinstance(..., TimeBase), also in binned.py, which at least locally fixed the failing doctest. The commit moving __add__, etc., is the most significant, as those did isinstance checks that could now be changed.

I think there is a bit more refactoring that could be done, given the code now in TimeBase that is looking for attributes like _delta_*, location, etc., but I think we might as well do that after, since it is a bit more intricate and keeping it there for now does not really change anything (the same could be argued for the move of _make_value_equivalent, which is after all simply overwritten in TimeDelta, but it is very Time-specific).

taldcroft · 2020-08-15T10:38:57Z

@mhvk - thanks for pushing forward with a bunch more changes. Alas I had made most of those changes already locally and was doing the last bit of tidy / double-checking but got stopped before I could push. I'm pretty sure the two versions of changes will be unreconcilable by automated means, but I'll have a look manually. One thing I did was look for all instances of isinstance\(.*, Time\) and evaluated how those should be. I do want to go back to (Time, TimeDelta) for reasons mentioned.

taldcroft · 2020-08-15T10:43:04Z

Reminder to open an issue about testing in TimeSeries @astrofrog. There were a couple of failures along the way that only showed up in the doc testing. IIRC the first was about initializing a TimeSeries with a TimeDelta.

mhvk · 2020-08-15T12:12:53Z

@taldcroft - do feel free to just remove my commits!

taldcroft · 2020-08-18T13:03:09Z

RTD fail seems unrelated:

WARNING: the readthedocs_ext.readthedocs extension does not declare if it is safe for parallel reading, assuming it isn't - please ask the extension author to check and make it explicit
WARNING: doing serial read

pllim · 2020-08-18T16:28:37Z

No, it is indeed not related. See #10668

- Initial refactor and one change with tests passing - Move Time-only methods into Time class - Export TimeBase and fix to_pandas() issue - Update changelog - Allow Time/TimeDelta to init from any Time/TimeDelta subclass - Fix issue initializing w/ TimeDelta in TimeSeries - Move _make_value_equivalent to be Time method

taldcroft · 2020-10-03T10:45:16Z

@mhvk - AFAIK all your comments have been addressed. Also from what I can see this entirely fixes #10654 so I updated the description accordingly.

In [2]: class TimeSub(Time): 
   ...:     pass 
   ...:                                                                                                                      

In [3]: ts = TimeSub('2000:001')                                                                                             
In [4]: ts                                                                                                                   
Out[4]: <TimeSub object: scale='utc' format='yday' value=2000:001:00:00:00.000>

In [5]: t = Time(ts)                                                                                                         
In [6]: t                                                                                                                    
Out[6]: <Time object: scale='utc' format='yday' value=2000:001:00:00:00.000>

In [7]: td = TimeDelta(1)                                                                                                    
In [8]: Time(td)  # Fail                                                                                                             
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

mhvk

@taldcroft - this looks very good indeed! I feel a bit bad about the coverage misses, but all were there before and are of the "obviously correct code" type, so might as well leave that for another time, when we just aim go get coverage of time up to 100%. So, merging!

taldcroft added time API change PRs and issues that change an existing API, possibly requiring a deprecation period labels Aug 14, 2020

taldcroft added this to the v4.2 milestone Aug 14, 2020

astropy-bot Bot added the table label Aug 14, 2020

mhvk reviewed Aug 14, 2020

View reviewed changes

mhvk force-pushed the time-base branch from 6c0a6ff to b5184ed Compare August 14, 2020 22:41

mhvk force-pushed the time-base branch from b5184ed to aef8ae1 Compare August 14, 2020 22:42

taldcroft force-pushed the time-base branch from aef8ae1 to 0290cc7 Compare August 18, 2020 11:43

taldcroft added the whatsnew-needed label Aug 18, 2020

dhomeier mentioned this pull request Aug 28, 2020

Excessive delay and memory usage when calling unsupported Numpy functions on TimeSeries.time #10688

Closed

taldcroft force-pushed the time-base branch from c3fe8a0 to d6bc4e9 Compare August 30, 2020 11:05

taldcroft added 7 commits October 3, 2020 06:34

Use (Time, TimeDelta) for isinstance checking not Time

84fa7f3

Update instance checking for arithmetic in time/core.py

2c37a03

Remove __rsub__ for Time

6fc6265

Move TimeBase __add/sub__ to Time

ada3941

Factor out common add/sub for TimeDelta

2317cbe

Move SCALES, FORMATS, to_datetime to Time class

2de914d

taldcroft force-pushed the time-base branch from d6bc4e9 to 2de914d Compare October 3, 2020 10:37

taldcroft requested a review from mhvk October 3, 2020 10:46

mhvk approved these changes Oct 3, 2020

View reviewed changes

mhvk merged commit d9ef063 into astropy:master Oct 3, 2020

taldcroft deleted the time-base branch October 5, 2020 09:26

This was referenced Oct 6, 2020

Time comparisons sometimes fail to fail #10651

Closed

Prevent comparisons between Time and TimeDelta from succeeding. #10652

Closed

Conversation

taldcroft commented Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

taldcroft commented Aug 14, 2020

Uh oh!

taldcroft commented Aug 14, 2020

Uh oh!

taldcroft commented Aug 14, 2020

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhvk Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

taldcroft Aug 15, 2020

Choose a reason for hiding this comment

Uh oh!

mhvk Aug 15, 2020

Choose a reason for hiding this comment

Uh oh!

pep8speaks commented Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2020-10-03 10:38:06 UTC

Uh oh!

mhvk commented Aug 14, 2020

Uh oh!

taldcroft commented Aug 15, 2020

Uh oh!

taldcroft commented Aug 15, 2020

Uh oh!

mhvk commented Aug 15, 2020

Uh oh!

taldcroft commented Aug 18, 2020

Uh oh!

pllim commented Aug 18, 2020

Uh oh!

taldcroft commented Oct 3, 2020

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

taldcroft commented Aug 14, 2020 •

edited

Loading

pep8speaks commented Aug 14, 2020 •

edited

Loading