-
Notifications
You must be signed in to change notification settings - Fork 147
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introducing Angle type [Issue #88] #169
base: main
Are you sure you want to change the base?
Conversation
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):
|
I have addressed your comments, I hope this is along the lines you envisioned.
You are right, thanks for bringing this up. I added more tests covering other special cases, eg. 0, 45, 180, 270 etc
They are now static functions.
🙈, my bad. You are absolutely right. I removed the hyperbolic functions. |
Tests/RealTests/AngleTests.swift
Outdated
} | ||
|
||
static func specialDegreesTrigonometricFunctionChecks() { | ||
assertClose(1, cos(Angle<Self>(degrees: -360))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests with exact results should test exact equality, not assertClose.
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 |
Ok, thanks for the effort |
The tangent tests are not passing, this should be fixed.
Why should |
Because |
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 |
A few thoughts: Why is there a constraint to Why is 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 |
True, it's removed now. Can't remember why I put it there in the first place.
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.
Ok, makes sense, will implement.
Correct, it's done. |
Sources/RealModule/Angle.swift
Outdated
@@ -160,6 +160,12 @@ public extension Angle { | |||
} | |||
} | |||
|
|||
extension Angle: Equatable { | |||
public static func == (lhs: Angle<T>, rhs: Angle<T>) -> Bool { | |||
lhs.radians.isApproximatelyEqual(to: rhs.radians) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementations of ==
cannot use approximate equality because it does not fulfill the semantic requirements of ==
.
Sources/RealModule/Angle.swift
Outdated
public static var zero: Angle<T> { .radians(0) } | ||
|
||
public static func + (lhs: Angle<T>, rhs: Angle<T>) -> Angle<T> { | ||
Angle(radians: lhs.radians + rhs.radians) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unfortunate to me that adding two angles specified in degrees incurs rounding error seven times if I want the result in degrees. I think this dedicated type needs to store values given in degrees as-is if it offers such arithmetic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I intended with my earlier comment, so degrees and radians are stored separately.
#169 (comment)
We could then switch on all operations performed on the input
and essentially handle three cases:
- degrees with degrees -> perform operation in degrees
- radians with radians -> perform operation in radians
- mixed units -> perform in a common unit
I'm not sure whether people agree on that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 That would yield a superior result in the case of operations performed entirely in degrees or radians.
I wonder if there are alternative designs that can improve the result when working with both degrees and radians. If the type stored both radians
and degrees
, for example, then you could store the result of "90 degrees plus 1 radian" exactly. I haven't thought through the remainder of the design, but I offer it for your consideration here.
Sources/RealModule/Angle.swift
Outdated
/// - Parameters: | ||
/// | ||
/// - range: The closed angular range within which containment is checked. | ||
func contained(in range: ClosedRange<Angle<T>>) -> Bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the value of these APIs over what's written in the body? I would suggest not including these separately; the existing forms are canonical and straightforward enough to write.
What @NevinBR is getting at, I think, is that 361 degrees is contained "between" 0 degrees and 2 degrees. This does not offer that functionality, and convenient access to it would be useful. It would need to be distinguished from the functionality here, though, through some thoughtful naming.
For API naming purposes, incidentally, the function would need to be named isContained
; and for style purposes, the access modifier public
should be specified on each member (and therefore is not necessary on the extension itself).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What @NevinBR is getting at, I think, is that 361 degrees is contained "between" 0 degrees and 2 degrees. This does not offer that functionality.
Additionally, 175º and -175º are both “between” 170º and -170º (but they are not between -170º and 170º).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, essentially you mean @NevinBR that the containment should be performed on the normalized angular range, right?
361º -> normalized to 1º -> contained in -10º...10º
361º -> normalized to 1º -> not contained in 10º...-10º
175º -> normalized to 175º -> contained in -170º...170º
-175º -> normalized to -175º -> contained in -170º...170º
175º -> normalized to 175º -> not contained in 170º...-170º
-175º -> normalized to -175º -> not contained in 170º...-170º
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not entirely sure how normalization should come into play. Probably yes normalized angles should be used for comparison / containment, and if someone wants to work with the unnormalized value they can extract it and do so manually.
However, several of your examples do not behave as I would expect:
175º -> normalized to 175º -> contained in -170º...170º
-175º -> normalized to -175º -> contained in -170º...170º
175º -> normalized to 175º -> not contained in 170º...-170º
-175º -> normalized to -175º -> not contained in 170º...-170º
These should be:
175º -> normalized to 175º -> not contained in -170º...170º
-175º -> normalized to -175º -> not contained in -170º...170º
175º -> normalized to 175º -> yes contained in 170º...-170º
-175º -> normalized to -175º -> yes contained in 170º...-170º
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. |
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 |
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. I'd personally lean more towards a wrapper of |
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. |
Wouldn't it make sense to only promise exact results for values created entirely from |
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 |
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:
There are some things that the above doesn't totally solve:
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 =) |
Fair enough. I currently see the following options moving forward:
Kindly let me know what you think. (@stephentyrone enjoy the family time :) ) |
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. |
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. |
I’m not sure I fully understand what is meant here. a. An Can @stephentyrone shed some light please? |
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:
I realize that this is not an especially clear answer. 🤷🏻♂️ Nonetheless, I think having your PR is great, because:
|
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. |
Hello, Any update on this? |
I don't think anything has changed since February. What are you hoping for? |
I was hoping for some feedback on whether merging the trig-pi functions branch was something we want to pursue or not. |
HI, why are the builds hanging? Do I need to do something? |
@swift-ci test |
Any feedback? @karwa / @davedelong (since you were involved in the discussion of Issue #88), or @danieljfarrell |
Anyone from the core team who would like to bother and give some feedback? |
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). |
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:
@NevinBR also offered a suggestion: 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 ..." |
Hi @stephentyrone, thank you very much for the elaborate reply.
I agree that this does not make much sense, my initial implementation was based only on radians.
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.
Agree, as explained above.
I don't have a primary use case to highlight, other than the implementation of Can we agree on the following?
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? |
I believe what @stephentyrone has been saying is that it is not possible to evaluate whether an 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. |
@stephentyrone Here is my implementation for the issue. Please let me know if I missed something.