Math operators for Angle class

[bryan-lunt]

It would be nice if there were operators in the Angle class, allowing us to add, subtract, even divide angles. Multiply by a float scalar, etc.

I will try to implement these myself, but a deep understanding of the API might be necessary.

Comparison operators would also be nice.

[EndlessDex]

If you are just trying to use the library:
The Angle class is a wrapper for a numpy array. So if you access the unit type (degrees/radians/etc) you can operate on the nunpy array and then shove it back into an Angle.

Oneliner example:
Angle(degrees=(arr_ang.degrees + 1))

If you're aware of all that and just want a new feature:
Adding that might be slightly tricky because of how the wrapper abstraction works. Some angles are arrays and some are single angles.

[bryan-lunt]

Thanks @EndlessDex , I am aware of how to deal with them, but I find that a pain and it makes the client code much more verbose.

numpy arrays and scalars already deal with this kind of complexity so it's not going to be difficult to implement. I think the most difficult thing will be dealing with the preference field of the Angle objects. Still, it will be doable.
My choice would be that if the preferences disagree, to take the one from the LHS.

[bryan-lunt]

Multiplying and dividing by unitless scalars makes sense.
The addition and subtraction operators should refuse to work except with other Angle objects, to prevent client code from accidentally trying to add radians to degrees.

[EndlessDex]

I take your point on ease of use, but disagree on how much of a pain it is. Relying on numpy or do numpy things and skyfield to do skyfield things is good practice.

Also, found that this request (made by you 5 years ago) has already been rejected? #646

[bryan-lunt]

Ooof. I'm getting old. I didn't remember asking for this. However, I should have searched for it.

[brandon-rhodes]

Welcome back, @bryan-lunt!

Thanks for providing the link to the earlier issue, @EndlessDex. 🙂

Last time I was fairly brief and hand-wavy in my reply. This time, let’s talk in more detail about what the cost would be of implementing operators. Here’s the kind of thing I imagine:

https://docs.python.org/3/reference/datamodel.html#emulating-numeric-types

class Angle:
    ...

    # 4 lines of code:

    def __add__(self, other):
        if not isinstance(other, Angle):
            return NotImplemented
        return Angle(radians=self.radians + other.radians)

    __radd__ == __add__  # maybe can just alias this? or even skip?

    # The __sub__ method will look similar,
    # except can’t just alias it to make it __rsub__,
    # so those two methods will be separate,
    # incurring maybe 8 lines of code.

    # 4 lines of code:

    def __mul__(self, other):
        if not isinstance(other, numbers.Number):
            return NotImplemented
        return Angle(radians=self.radians * other)

    __rmul__ == __mul__  # maybe can just alias this too?

    # Then, __truediv__ has to handle both scalar,
    # but also other Angle as valid cases, so probably
    # at least 6 lines of code.

    # And 6 more lines for __rtruediv__.

    # Can probably skip __floordiv__ for an angle?

    # what about __mod__ and __divmod__?
    # what about __pow__?

    # Then, 2 lines of code each for __neg__, __pos__,
    # __abs__, __invert__.

So I’m seeing at least 36 lines of code required to turn Angle into a math-capable class.

But there are complications.

You have already mentioned that angles have a display preference. So the above code would need a couple of more lines to copy the preference from the LHS or RHS.

But, also, the results of, say, addition, will be wonky given the above code, if the user specified degrees: an angle with 10.0 degrees added to an angle of 20.0 degrees might produce an angle that’s not quite 30.0 degrees because the underlying math will use radians. The correct thing would be to teach angles to remember if they were created using degrees, so that they could then add the degrees together directly in __add__() and ignore the radians — which would result in less user complaints and questions, at the expense of more lines of code.

Also, as I guess I mentioned in the earlier issue, these operations will run more slowly than if the user simply adds the degrees or radians manually, because of all the typechecks that need to happen. But maybe that doesn’t really matter, and users will never notice the small delay, given how much other expense is usually involved in computing positions? On the one hand, if they say ‘it’s slow!’, we could always say, then add .radians manually and skip the checks. On the other hand, that’s yet another thing to document and answer questions about. Again, maybe not a real issue, but more an issue in my imagination.

Then we have to think about other units.

What about classes other than Angle?

My assumption is that users will also need math methods added to the other unit classes, since otherwise they won’t understand why one kind of unit supports addition but another one doesn’t — so the above logic would need to be added to Distance, Velocity, and AngleRate, at a cost of 3 × 36 = 108 more lines of code, for a total of 144 new lines of code.

It might be urged that all those unit classes inherit from Unit and so maybe the methods only need to get added once, up in Unit, by doing the checks generically by saying something like isinstance(other, type(self)) instead of something specific like isinstance(other, Angle).

In that case the expense would only be one class’s worth of dunder methods, not four.

A worry I have, as the maintainer, is that these methods would have dozens of hiding corner cases that would lead to users opening issues about what, say, multiplication is doing in edge cases; and that maybe there would be more dunders that folks would expect once the simple ones like addition and multiplication were supported.

And when folks come along with, say, detailed reasoning about the situations in which an Angle can and cannot be raised to a power, will I be able to say ‘no’ if I said ‘yes’ to so many other dunders already?

Oh, I forgot comparisons. They are listed partway down in this section of the docs:

https://docs.python.org/3/reference/datamodel.html#basic-customization

That would be three or six additional methods, depending on whether Python behaved well if __ge__ wasn’t defined but its opposite __lt__ was. So maybe 24 lines of code.

Having sketched out all of the above, I suppose I should add some perspective:

The units.py module is currently 467 lines long (ignoring blank lines).

If I could really accomplish this up in Unit, writing it only once instead of once for each kind of unit, and it cost 36 + 24 lines for the basic math operators plus the comparisons, then the 60 new lines of code would be about a 12% increase in the size of the module.

Which is a stiff penalty, but not unimaginably stiff.

But it’s more complexity, more maintenance going forward, and in all of the years Skyfield has been available, I think these two issues are the only ones that have asked for this.

So I’m tempted to spend my coding time on other issues?

But I’m also tempted because this would be a nice feature.

But it feels like a spiky problem with lots of edge cases.

Hmm.

[EndlessDex]

The complaint seems to be based on the verbosity of math with Units. A compromise could potentially be adding compatibility with the numpy math functions (ufunc).
Something like:

x = Angle(radians=[0.0, math.pi, 2.0 * math.pi])
x = np.add(x, math.pi / 2.0)

The first part of this page covers how to do so.
https://numpy.org/doc/stable/user/basics.dispatch.html

Of course this requires relying on the underlying radian storage. And you still have to choose how to port the metadata for left and right-handed instances. But it does mean you're not implementing all the code yourself.

Comparability with the standard dunders could also be added but it requires inheriting from a numpy class to do it easily.

[bryan-lunt]

Thank you for the considered reply. It was a good read, and you're (@brandon-rhodes ) one of the people I want to learn (more) software engineering from.

As a user, at least for myself, I don't think I'd care about rounding errors in the degrees. This is because I know that the fundamental unit is radians and it's immediately turned to radians at instantiation.

If someone cared about several decimal places of degrees, then the Angle class would have to have more than a preference, its internal storage would have to be able to change to be sometimes radians and sometimes degrees. (Maybe it does that and I didn't read it carefully enough?)

Having already made some design choices, it is necessary to continue to honor them to avoid breaking client code.

I might have done it differently (I absolutely do not mean to say that my way is "right"):

I think I'd have had an abstract base class for Angle and then two kinds of concrete angle classes that are interoperable with each other. One with a radians and one with a degrees storage class.
Or, to hide that from the user, maybe an Angle class with a private inner class AngleStorageAbstractBase which has two concrete subclasses.

But the choice to just store everything as radians does away with all of these needs. I think it's a good choice.

Your code is good and I think I understand your choices, but often when I see code that looks like "If I should behave as an X... if I should behave as a Y..." I start to wonder if that should not have been inheritance instead of flow control.

I know "everyone's a critic" as the saying goes. As the author you have a deeper understanding of why your choices were necessary.

And yes, I was also going to ask for comparison operations.
If all else were equal, and it would not create too much maintenance, I would like to be able to add, subtract, multiply, and compare things which are conceptually numbers using simple syntax.

[bryan-lunt]

If I ask for it again in another 5 years, it really will be because my memory is going, not because I'm trying to break down your resolve.

[lll123github]

I am considering whether add operation can be used in Angle class to complete my coordinate conversion method one month ago, now I see this issue. :)