Remove AngularSeparation as a distinct class - #1007

+1 to remove AngularSeparation as a separate class.
I can make a case if others think it should be kept.

+1

It was there to disallow certain operations. If you get the separation between two points on the sky, and then add that to one of them, that is an ambiguous operation. It would be allowed if all were Quantities, but would be a different operation than intended.

@demitri - that's what I meant with the "vector" mention in the description. But consider this scenario: I want to know the angular separation from object A to object B which gives me an AngularSeparation (sep = c1.separation(c2)). But then I want to know what half that separation is, so I do sep/2 as I've done many times with Angle objects. Whoops, that's an error, because AngularSeparations support no operations. Similarly, maybe I want to know the total angular distance from A to B to C, so I do c1.separation(c2) + c2.separation(c3) ... again, an error, even though the request is well-defined and quite natural.

So I guess while I originally saw the intent here, I now see other use cases that we are currently disallowing that make perfect sense.

I don't feel strongly about this. As long as we have our bases covered, I'm ok with removing this.

Just making a note to follow up on this after #1006 is merged.

See #1006 - I think that we may want to think a bit more about this before removing it. In particular, in the Time class, we have kept a distinction between an absolute time and a time interval, and it may make sense here to also distinguish between an absolute angle and the difference between two angles.

I think that angle is fundamentally a relative quantity (unlike Time) so I'm not sure what AngularSeparation brings that is needed. To me an angle is the analog of TimeDelta.

Of course you can always define a 3-vector which represents that rotation between two absolute points on the sky.

@taldcroft - I don't think it's quite true that Angle is completely relative because the concept of bounds complicates things. For instance, if you have two angles with bounds of [-45. 45] how should they behave when subtracted? The current behavior is actually wrong:

In [6]: a1 = Angle(-44., bounds=[-45., 45.], unit=u.degree)

In [7]: a2 = Angle(+44., bounds=[-45., 45.], unit=u.degree)

In [8]: a1 - a2
Out[8]: <Angle -88.00000 deg>

In [9]: a = a1 - a2

In [10]: a.bounds
Out[10]: [-45.0, 45.0]

because the value is out of bounds. But then, maybe the result shouldn't have bounds because it's a difference, not the absolute angle. Do you see what I mean? Maybe we don't need AngleDelta, but we need to be smarter about when the bounds are carried over to the result (which indicates an absolute angle), and when they are not (which indicates a relative angle).

If an Angle has bounds, you can only perform a mathematical operation on it with another Angle of the same bounds, so it's reasonable to get an Angle with the same bounds as a result. Given that, the result to the above should throw an out-of-bounds error.

Does Angle absolutely need bounds? To me this is mostly a convenience for the output representation of an angle and 95% of the time it will be -180 to +180 or 0 to 360. If the bounds end up causing problems or exceptions for what should be straightforward operations then something is wrong. I definitely would not (as a user who is trying to get work done) want to see an exception in the example you showed.

Apologies if I'm missing something obvious, I'm still only on cell phone and can't play around with code.

I still think of angle as a relative value just like distance. It's only in the context of a coordinate that there is a well defined origin which anchors an angle or distance.

For latitude, the bounds are currently -90 to 90, which is not the full range of angles, and this is where the question of how to deal with the output bounds comes in. Anyway, the most basic issue here is how ufuncs should behave with Angle, and we should discuss this more in #1006. I just wanted to put a note here to say that we should hold the decision until we know how what behavior we want in #1006.

If the proposal to drop bounds from Angle is accepted (#1310) then I think this becomes an easy decision since AngularSeparation would basically just be a crippled Angle.

There is a benefit for users in not over-specializing. If user asks for the separation between two coordinates they expect a simple angle object that supports normal operations. Making them learn about yet another subclass which doesn't do anything but restrict those operations doesn't seem like the best path.

@eteq @astrofrog - I've attached code that removes the AngularSeparation class.

@taldcroft - looks great! Just one small comment above.

This probably warrants an entry in the changelog?

The consensus seems to be in that it's fine to remove this, it's pretty straightforward, and the tests are passing, so I'll just go ahead and merge this now. There's the inline question by @mhvk and @astrofrog about the proper to behavior for Quantity and Angle, but changing that behavior definitely belongs in a separate issue/PR.