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

[jkalias]

Any feedback? @karwa / @davedelong (since you were involved in the discussion of Issue #88), or @danieljfarrell

[jkalias]

Anyone from the core team who would like to bother and give some feedback?

[jkalias]

I have tried several times to get some feedback about this PR, and what I can do to improve things. Unfortunately, I have the impression I am talking to a wall, and I see no interest. Perhaps this PR is now completely irrelevant with the introduction of the Spatial framework (?).

In any case this is not the kind of feedback/experience I was hoping for or the one which is being advertised by Apple ("We value our community efforts" and such).

[stephentyrone]

Hi @jkalias --

I (and others) have laid out some concerns about representation that would prevent taking the PR as is upthread, but I'll summarize them here for convenience:

• Storing degrees and radians separately doesn't make much sense to me; it makes arithmetic more complicated to implement correctly for minimal benefit.
• Storing either degrees or radians is flawed because common values in either one do not have exact representations in the other.
• Storing "turns" or "half turns" (radians with an implicit scale of π) is probably the correct choice for a general-purpose angle type, but this requires some additional supporting work (e.g. the trig-pi functions, which you've pulled in here, and accurate stand-alone angle reductions, which don't currently exist anywhere).

@NevinBR also offered a suggestion: 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.

More broadly there's a question of where this type belongs: is it appropriate for Swift Numerics at all, and if so, what module does it belong in? It is somewhat more niche than the operations already defined in RealModule, or at least relevant to a different audience, but neither is it obvious that they don't belong there. More broadly still, there's a question of how "unit-like things" should be handled in Swift, and how angles fit into that, which also fits into the need for use cases. It's hard to answer these questions without seeing how the type is actually used--for this purpose, it would be nice to be able to point to this or another prototype on a branch or clone that people are actually using. Do you have examples that you can share that use your branch or another implementation? Does someone else? It would be really nice to be able to say something like "I've been using this prototype for a few months to do X, and it's great, but it would be even better if ..."

[jkalias]

Hi @stephentyrone, thank you very much for the elaborate reply.

• Storing degrees and radians separately doesn't make much sense to me; it makes arithmetic more complicated to implement correctly for minimal benefit.

I agree that this does not make much sense, my initial implementation was based only on radians.

• Storing either degrees or radians is flawed because common values in either one do not have exact representations in the other.
• Storing "turns" or "half turns" (radians with an implicit scale of π) is probably the correct choice for a general-purpose angle type, but this requires some additional supporting work (e.g. the trig-pi functions, which you've pulled in here, and accurate stand-alone angle reductions, which don't currently exist anywhere).

My understanding is that it would be beneficial to store the "turns" or "half-turns" in either degrees or radians (using a flag/enum to signify which case it is), but then we will "suffer" the accuracy loss when converting between the types. If we accept the trig-pi function merge, we can at least make sure that we have better results when evaluating the trig functions. Regarding the argument reduction part, I have no idea how something like this can be tackled, perhaps someone else better suited can tackle this in a next iteration step.

@NevinBR also offered a suggestion: 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.

Agree, as explained above.

More broadly there's a question of where this type belongs: is it appropriate for Swift Numerics at all, and if so, what module does it belong in? It is somewhat more niche than the operations already defined in RealModule, or at least relevant to a different audience, but neither is it obvious that they don't belong there. More broadly still, there's a question of how "unit-like things" should be handled in Swift, and how angles fit into that, which also fits into the need for use cases. It's hard to answer these questions without seeing how the type is actually used--for this purpose, it would be nice to be able to point to this or another prototype on a branch or clone that people are actually using. Do you have examples that you can share that use your branch or another implementation? Does someone else? It would be really nice to be able to say something like "I've been using this prototype for a few months to do X, and it's great, but it would be even better if ..."

I don't have a primary use case to highlight, other than the implementation of Angle I committed for Euclid. My initial goal with this PR was to provide a "correct" and "safe" way to do angle math; in my professional experience I have been scorched several times by APIs which expect an angle in double. I would like to use the compile to enforce correctness so it's impossible to do "wrong" angle operations; thereby adhering to Swift's primary goal of being a safe language :).

Can we agree on the following?

1. Use half-turns as the backing store for Angle, with a corresponding flag to indicate degrees or radians
2. Accept some loss when converting between degrees and radians (if stored by the "other" type) by multiplying by "pi/180" or "180/pi" accordingly
3. Use the trig-pi functions with trig operations
4. Leave the argument reduction for a next PR

If not, I think I should just delete this PR since I don't want to waste everybody's time for nothing.

What do you think?

[xwu]

I believe what @stephentyrone has been saying is that it is not possible to evaluate whether an Angle type should or shouldn't be included as part of this library without first having an implementation of all the building blocks, which includes argument reduction.

Once users have all the building blocks available to them, then it will be possible to consider whether additionally having this type will provide meaningfully more benefit or whether users can get most or all of those benefits by writing simple code that directly calls the building blocks themselves.

For this reason, having this PR here as a record of all of these discussions is helpful and it doesn't need to be closed.