Introducing Angle type [Issue #88]

[jkalias]

@stephentyrone Here is my implementation for the issue. Please let me know if I missed something.

[stephentyrone]

There's a few things that I would want to resolve before merging this (this is based on a quick skim, rather than a detailed review):

• It doesn't implement the semantics that one would want of an Angle type; for example, Angle(degrees: 90).cos should be exactly zero, but it is not with this implementation.

• It's a rather large departure from the existing APIs for ElementaryFunctions and normal mathematical notation in making them properties instead of static functions (.cos(angle)) or free functions (cos(angle)).

• It's not obvious that the hyperbolic functions really makes sense for an angle type (there's no "angle" in the usual geometric interpretation of these functions to take in degrees or radians, rather an area).

[jkalias]

I have addressed your comments, I hope this is along the lines you envisioned.

There's a few things that I would want to resolve before merging this (this is based on a quick skim, rather than a detailed review):

• It doesn't implement the semantics that one would want of an Angle type; for example, Angle(degrees: 90).cos should be exactly zero, but it is not with this implementation.

You are right, thanks for bringing this up. I added more tests covering other special cases, eg. 0, 45, 180, 270 etc

• It's a rather large departure from the existing APIs for ElementaryFunctions and normal mathematical notation in making them properties instead of static functions (.cos(angle)) or free functions (cos(angle)).

They are now static functions.

• It's not obvious that the hyperbolic functions really makes sense for an angle type (there's no "angle" in the usual geometric interpretation of these functions to take in degrees or radians, rather an area).

🙈, my bad. You are absolutely right. I removed the hyperbolic functions.

[stephentyrone]

This approach doesn't work to make exact cases of degrees exact (or rather it does, but unfortunately introduces enormous errors for radian arguments; this would make cos(Angle(.pi/2)), which should be non-zero, return zero. This is solvable, but not by massaging the trigonometric functions themselves; it has to be handled in the angle initializer instead. I'll put together a sketch showing how to do it correctly.

[jkalias]

Ok, thanks for the effort

[jkalias]

This approach doesn't work to make exact cases of degrees exact (or rather it does, but unfortunately introduces enormous errors for radian arguments; this would make cos(Angle(.pi/2)), which should be non-zero, return zero. This is solvable, but not by massaging the trigonometric functions themselves; it has to be handled in the angle initializer instead. I'll put together a sketch showing how to do it correctly.

Why should cos(Angle(.pi/2)) not be zero by the way??

[stephentyrone]

Because .pi is not exactly π (π is not exactly representable in any FloatingPoint type).

[jkalias]

What would you think of another approach: keeping track of which initializer was used like this

public struct Angle<T: Real & BinaryFloatingPoint> {
    public var radians: T {
        switch input {
        case let .radians(radiansValue):
            return radiansValue
        case let .degrees(degreeValue):
            return degreeValue * Angle.radiansPerDegree
        }
    }
    
    public init(radians: T) { self.input = .radians(radians) }
    public static func radians(_ val: T) -> Angle<T> { .init(radians: val) }
    
    public var degrees: T {
        switch input {
        case let .radians(radiansValue):
            return radiansValue * Angle.degreesPerRadian
        case let .degrees(degreeValue):
            return degreeValue
        }
    }
    public init(degrees: T) { self.input = .degrees(degrees) }
    public static func degrees(_ val: T) -> Angle<T> { .init(degrees: val) }
    
    private enum Input {
        case degrees(T)
        case radians(T)
    }
    
    private let input: Input
    
    private static var degreesPerRadian: T { 180 / .pi }
    
    private static var radiansPerDegree: T { .pi / 180 }
}

Then in the trig functions, we could switch on the .input: if radians, then use directly the corresponding trig function. For degrees, we could try to find if the input is an integer multiple of 30deg and/or 45deg and decide accordingly.

[NevinBR]

A few thoughts:

Why is there a constraint to BinaryFloatingPoint? Is Real not sufficient?

Why is normalize a private free function, rather than a public instance method normalized()?

Some applications need to represent angles greater than a full rotation.

One of the common things I want to check is “Does angle α fall between angles β and γ?” Or similarly, “Is angle δ no more than angle ε away from angle ζ?”

Shouldn’t Angle conform to AdditiveArithmetic? And allow scalar multiplication?

[jkalias]

A few thoughts:

Why is there a constraint to BinaryFloatingPoint? Is Real not sufficient?

True, it's removed now. Can't remember why I put it there in the first place.

Why is normalize a private free function, rather than a public instance method normalized()? Some applications need to represent angles greater than a full rotation.

This was done in the effort to massage the trigonometric functions, as @stephentyrone pointed out in an earlier comment, so that angles initialized with special values for degrees (eg. 90) give exact results. @stephentyrone mentioned though that this approach is not right, and he will prepare a draft of what he thinks is the appropriate solution path, so I am waiting for this.

One of the common things I want to check is “Does angle α fall between angles β and γ?” Or similarly, “Is angle δ no more than angle ε away from angle ζ?”

Ok, makes sense, will implement.

Shouldn’t Angle conform to AdditiveArithmetic? And allow scalar multiplication?

Correct, it's done.

[jkalias]

I tried to address all issues which came up during the previous round of discussion.

1. Degrees and radians are stored separately, so Angle(degrees: 20) + Angle(radians: .pi) will accurately result in 200°
2. Exact trigonometric results (cos, sin and tan) for known degrees are now returned
3. Range detection is implemented as discussed previously based on the normalized angles
4. Distance from given angle is implemented (“Is angle δ no more than angle ε away from angle ζ?”)

I'd be interested to know what the community thinks about this approach, or whether this is totally in the wrong direction.

[NevinBR]

I’m not convinced storing both degrees and radians separately is worthwhile. I would lean more toward a design that stores a value (of type T) and a unit (probably a resilient enum). If we’re certain we’d only ever want to model degrees and radians then it could be a simple “isInDegrees” flag.

That way any operation on two values with the same unit, can be performed directly on the stored value. If the units are different, then I think the result should probably be stored as radians.

The range-containment stuff can be written a bit more simply:

if normalizedStart <= normalizedEnd {
  return (normalizedStart...normalizedEnd).contains(normalizedAngle)
} else {
  return !(normalizedEnd...normalizedStart).contains(normalizedAngle)
}

I’m not sure that “isClose” really carries its weight. Being able to use “isInRange” should should suffice.

[jkalias]

I’m not convinced storing both degrees and radians separately is worthwhile. I would lean more toward a design that stores a value (of type T) and a unit (probably a resilient enum). If we’re certain we’d only ever want to model degrees and radians then it could be a simple “isInDegrees” flag.

That way any operation on two values with the same unit, can be performed directly on the stored value. If the units are different, then I think the result should probably be stored as radians.

How about then storing the angle using foundation (Measurement<UnitAngle>(value: 30, unit: .degrees)). It already provides a wealth of conversion utilities.

My biggest concern in this implementation was the trigonometric functions. I wasn't sure if it's something acceptable or just a "hack".

[jkalias]

Hello and happy/healthy new year to everybody.

It's not clear to me if there is something I need to do here, or if people are still thinking about it. Any feedback would be deeply appreciated.

[danieljfarrell]

I don’t like this at all.

It introduces additional complexity for very little benefit. I much prefer keeping the types simple as possible and making the functions well documented or with function signatures which make this obvious.

Generally everything should be in radians in numerical code. I’m all for providing convenience functions which convert between angular definitions; such as in numpy https://numpy.org/doc/stable/reference/generated/numpy.degrees.html

[jkalias]

I am all in for radians in numerical code. However, judging from previous comments, it seems that the community does not accept inaccuracies for degrees inputs (eg. cos(Angle(degrees: 90)) != 0).

I'd personally lean more towards a wrapper of Measurement<UnitAngle> which already handles transformations between various inputs, and then explicitly using the radians value in trigonometric functions.

[danieljfarrell]

Yes, that’s the kind of thing that people just starting out with numerical code might find surprising. Anyone with some some experience or understanding of floats will accept this, and is aware of it.

Who is the intended audience of the library? If it’s to encourage swift as a machine learning, data, scientific programming language, then sticking to conventional numerical programming norms is much more likely to get this adopted and used.

[superlopuh]

This approach doesn't work to make exact cases of degrees exact (or rather it does, but unfortunately introduces enormous errors for radian arguments; this would make cos(Angle(.pi/2)), which should be non-zero, return zero. This is solvable, but not by massaging the trigonometric functions themselves; it has to be handled in the angle initializer instead. I'll put together a sketch showing how to do it correctly.

Wouldn't it make sense to only promise exact results for values created entirely from Angles? If the values are represented as turns, then cos(Angle.pi / 2) should be exact, since Angle.pi would be represented as 1.0 or 0.5 internally, depending on whether the backing range is 0..<1 or -0.5..<0.5 or -1..<1. (I would prefer the spelling Angle(turns: 0.5)) Even if the internal representation is different, it seems like promising exact results for values that have been converted from floating-point representations will be difficult in the general case.

[jkalias]

I think we need to first answer what @danieljfarrell mentioned: what is the primary use case of swift-numerics and what is the target audience.

According to the introduction section, "Swift Numerics provides a set of modules that support numerical computing in Swift". My experience with numerical computing is using exclusively radians, which is reflected in my first PR.

However it seems @stephentyrone has some objections regarding inaccuracies for special degrees cases. I have also never heard of turns in numerical code, but I'm definitely not an authority on the issue (and it's not my intention to sound like one)

[stephentyrone]

My goal for Swift Numerics is to guide people to solutions that eliminate as many sources of error as possible, and to reduce errors that cannot be eliminated. Conversion from degrees (or turns) to radians for use with trig functions is one such common source of error. From that perspective, it makes good sense to provide functions and/or types that make it easier to avoid error--but only if they do not introduce new errors instead. It's the second half of this that makes "simple" solutions to this problem a little bit tricky.

Tricky, but not impossible; there are a few different ways in which this can be achieved, with slightly different tradeoffs. My sketchy design for a solution right now is along the following lines:

• internally angles can be represented in fractions of π from -1 ... 1.
• initialization from degrees is pretty easy ("just" use formRemainder).
• initialization from radians requires an "infinite π" reduction. This is not hard for any concrete type (see Payne & Hanek's original paper, or K.C. Ng's more approachable "ARGUMENT REDUCTION FOR HUGE ARGUMENTS: Good to the Last Bit"). It is quite a bit more subtle to implement for arbitrary floating-point types, so a generic implementation is hard (and requires a lot of support machinery that doesn't exist in Swift or Swift Numerics yet). So this would probably have to be a protocol requirement on Real (which is fine, we can do that). Exact signature to be worked out.
• the above together with "trig-pi" functions gives a complete solution to the simplest form of the problem ("sin(180˚) should be zero").

There are some things that the above doesn't totally solve:

• Angles are equivalence classes so integer numbers of turns are lost. I think that's appropriate, but it's not what some people will want.
• There's much more relative accuracy close to zero than close to +/-1. This means that angles close to (4k+{1,2,3})π/2 get less accuracy than angles close 2kπ for integer k. This can be "solved" by representing the angle as a quadrant and offset, but that is not a complete solution, because if θ is unusually close to 2kπ + π/4, then 2Angle(θ) would be computed with much less accuracy than Angle(2θ). This can't really be solved without arbitrary-precision, which is a thing we might do someday, but an Angle type shouldn't be dependent on it, so I'm provisionally calling it out-of-scope.

All of which is to say that I have a fairly complete solution worked out in my head, but I've been on new parent leave for the last little while and therefore mostly not working on such things =)

[jkalias]

Fair enough.

I currently see the following options moving forward:

1. I pause this PR and step down till @stephentyrone (or someone else ?) has more time to implement his approach
2. I merge #85 in my branch so I have access to "trig-pi" functions and then implement a draft of the "conversion from degrees" part. The "conversion from radians" will be implemented in a second step.
3. Other ideas from the community?

Kindly let me know what you think.

(@stephentyrone enjoy the family time :) )

[danieljfarrell]

I'm not disputing these are useful features, just that, I don't think they belong in a numerics library.

I would expect a symbolic mathematics package to correctly evaluate these as you have outlined. But numerics is not symbolics.

If we look at ecosystems that have been successful, let's take Python for an example.

numpy is fast and efficient because it's all build around a simple data structure NDArray. The core does not try to do too much other than provide useful vectorised array operations and numeric types (there are useful subpackages for handling various numerical tasks)

As numpy became ubiquitous in the Python community (it's used by over 500,000 open source projects) various packages have emerged which provide useful domain specific behaviour. This rich ecosystem is great for developers, but it is enabled by a rock solid numerical library.

My concern here is that numerical library should be fairly simple and not abstract away, or try and make floating point operations nice and tidy; because they are not. It feels almost like an anti-pattern.

I think the functionality described here is better as a third party project which has a dependency on swift-numerics, rather than being incorporated into it.

[stephentyrone]

My concern here is that numerical library should be fairly simple and not abstract away, or try and make floating point operations nice and tidy; because they are not. It feels almost like an anti-pattern.

I agree, but that's the exact opposite of what's being discussed here. Rather, my goal is to pare down complex numerical algorithms to the core components that must be implemented by experts; the things that there should be a single high-quality implementation of so that all the other projects do not need to implement them themselves. An angle type isn't that, but the building blocks that make it possible (infinite-precision π/2 reduction, trig-pi functions) are.

[jkalias]

I’m not sure I fully understand what is meant here.

a. An Angle type should not be defined in swift-numerics because it’s not in the intended scope of the library. b. The Angle type should be included in swift-numerics based on the building blocks of infinite-precision π/2 reduction and trig-pi functions.

Can @stephentyrone shed some light please?

[stephentyrone]

The building blocks for an angle type absolutely should be defined in Swift Numerics. The angle type itself might or might not be, but it almost doesn't matter either way because:

• once you have the right building blocks, providing the type is "trivial", so another project can easily do it.
• once you have the right building blocks, providing the type is "trivial", so there's no downside to including it in Swift Numerics.

I realize that this is not an especially clear answer. 🤷🏻‍♂️

Nonetheless, I think having your PR is great, because:

• it lets us discuss this, which obviously needs to happen.
• it provides an opportunity to iterate on and flesh out what an Angle API might look like, which is useful even if it ends up living somewhere else.
• it provides a good place to discuss implementation details so we can figure out more precisely what building blocks are needed.

[jkalias]

Following the previous discussions, I made an attempt this time using the trig-pi functions introduced in branch #85. I still don't have a working method for a correct argument reduction (especially for large arguments). I kept the private backing fields of both degree and radian parts, in my attempt to increase the accuracy in addition/subtraction.

[jkalias]

Hello,

Any update on this?

[stephentyrone]

Any update on this?

I don't think anything has changed since February. What are you hoping for?

[jkalias]

I was hoping for some feedback on whether merging the trig-pi functions branch was something we want to pursue or not.

[jkalias]

HI, why are the builds hanging? Do I need to do something?

[jkalias]

@swift-ci test