Add isclose() method to Time and TimeDelta classes - #10646

There should be no need to do that - TimeDelta should be smart enough! (And I think it is - if not, that's a bug...)

I tried the original version first and it revealed the bug. I'll open an issue, but in the meantime the workaround code is needed.

In [2]: dt = TimeDelta([-1, 1]*u.s)                                                                                                                           

In [3]: np.abs(dt)                                                                                                                                            
Out[3]: 
array([<TimeDelta object: scale='None' format='jd' value=1.1574074074074073e-05>,
       <TimeDelta object: scale='None' format='jd' value=1.1574074074074073e-05>],
      dtype=object)

See #10653

As noted there, plain abs(...) should work.

I'm wondering whether the checks are necessary, since ideally the comparison just breaks. Indeed,

In [20]: TimeDelta(1*u.day) < Time.now()                                                              
TypeError: '<' not supported between instances of 'TimeDelta' and 'Time'

But very strangely,

In [21]: TimeDelta(1*u.day, scale='tai') < Time('J2000')                                                                            
Out[21]: True

It looks like it has something to do with the specific scale picked - I'll investigate.

Ignoring that, would it be an idea to remove the checks? It might allow Time and TimeDelta can share the implementation.

I favor a more verbose implementation that does input validation up front and gives a clear message about what is wrong. With your version the user is likely to not immediately know what the problem is. IMHO we need more explicit validation in astropy.

I guess it is the usual "look before you leap" vs "easier to ask for forgiveness than permission" - https://docs.python.org/3/glossary.html#term-eafp

Generally, python seems to prefer the latter, and I think it is not illogical here either. E.g., suppose someone writes a different MyTime class (say, one that is integer + fraction in seconds), which they try to ensure can interact with Time, e.g., by defining subtraction with astropy's Time (which they can make work, because python will give their __rsub__ a chance after our __sub__ fails). If this also returns something that can have its absolute value taken and be compared with a quantity or TimeDelta (as they might well, since those are methods we've had for a long time), then the actual isclose() code will "just work". But in the current implementation, the isinstance check makes it impossible to ever work. (This is similar to numpy having np.asarray sprinkled throughout their code, making it not work with Quantity, and indeed np.abs sort-of working rather than just plain failing).

p.s. In principle, in EAFP, one does try/except and gives perhaps more informative error messages, but I've slowly moved to a state where I think one should just count on the user looking at the docstring when an error is raised. And in this case, one would generally get a quite informative TypeError from the subtraction anyway.

I see your point and it is valid. OTOH, it happens frequently (based on real astropy issues I've seen) that users get confused because of an obscure error message raised in an unrelated bit of code when they pass a bad input. So there is a balance between addressing a problem that it almost sure to occur (bad inputs) vs. a new MyTime class that is less likely.

It's also worth noting that a major theme of recent Python development is type annotation. I've actually started paying some attention to this since there must be a reason the core devs are spending so much time on it. From this perspective we would annotate the input with something. Agreed that I'm not sure how this really works with duck-typing, which is certainly an incredibly useful concept.

I guess the middle ground is putting the main bit of the function in a try / except TypeError and re-raising with a better exception.

Agreed that the MyTime class is not very likely, and I should add that this is not a big deal! But if one does it without anything, the type of exception one gets is already quite clear:

In [11]: class TTime(Time): 
    ...:     def isclose(self, other, atol=1*u.ns): 
    ...:         return abs(self - other)<atol 
    ...:               
    ...:                                                                                               

In [12]: t = TTime.now()                                                                               

In [13]: dt = TimeDelta(100, format='jd')                                                              

In [14]: t.isclose('J2000')                                                                            
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-14-15f66b730fe3> in <module>
----> 1 t.isclose('J2000')

<ipython-input-11-b8b243443d6c> in isclose(self, other, atol)
      1 class TTime(Time):
      2     def isclose(self, other, atol=1*u.ns):
----> 3         return abs(self - other)<atol
      4 
      5 

TypeError: unsupported operand type(s) for -: 'TTime' and 'str'

In [15]: t.isclose(dt)                                                                                 
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-15-f21ef40edf4f> in <module>
----> 1 t.isclose(dt)

<ipython-input-11-b8b243443d6c> in isclose(self, other, atol)
      1 class TTime(Time):
      2     def isclose(self, other, atol=1*u.ns):
----> 3         return abs(self - other)<atol
      4 
      5 

TypeError: bad operand type for abs(): 'TTime'

p.s. The annotations are indeed intriguing. I think currently they are only to help writing correct code, not execution, but I think it is a good way to make the intent clearer. On annotations, our very own quantity_input decorator is an exception in that it actually uses annotations at runtime. Actually, that is seeing some nice discussion over at #10156 with a draft new implementation at #10655 - the latter might even get to your long-standing suggestion of having a way to ask for Quantity input that gives the function plain arrays in the right units! Would be good to get your input!