swift-numerics icon indicating copy to clipboard operation
swift-numerics copied to clipboard

Rename halfAngle to argument and make it public

Open markuswntr opened this issue 2 years ago • 3 comments

As of now, Quaternions provide a number of different representations to access the encoded rotational/transformational properties, alongside a function to directly act on (rotate) an arbitrary 3 dimensional vector. Two of these representations are angle-axis and polar coordinate form. Both of them are defined, not just for unit quaternions, but for any arbitrary finite and nonzero quaternion and they both have very similar attributes, i.e.: they both are defined by the length of the quaternion, the axis of rotation and an angle alongside the axis. However, the definiton of these angles is different for polar (in [0, π] range) and angle-axis (in [0,2π] range), and while access to the angle of the angle-axis representation has been made publicly available via the angle property, the (half)angle of the polar form has not been made publicly available to avoid unnecessary churn in the API surface (though it has been available internally to the module via halfAngle). The only way to (publicly) access the angle of the polar form is through the phase value of the polar tuple property, which also yields length and axis.

This PR tries to address this issue, by renaming halfAngle to argument, and making it publicy available. Additionally, for consistency, it removes references to halfAngle through phase and replaces them with argument. The later, however, does require 2 changes in the public API:

  • The polar initializer has been renamed: public init(length: RealType, phase: RealType, axis: SIMD3<RealType>) -> public init(length: RealType, argument: RealType, axis: SIMD3<RealType>) This could be solved by providing both, and deprecating the first one.

  • The polar tuple has been changed (phase -> argument): public var polar: (length: RealType, phase: RealType, axis: SIMD3<RealType>) -> public var polar: (length: RealType, argument: RealType, axis: SIMD3<RealType>) This can not be solved by deprecation (AFAIK), and would be a source breaking change.

Given that quaternions are not yet part of the main branch, I have not yet addressed these 2 issues, and both would be source breaking. But I am definitely willing to add deprecations or change the PR if these source breaking changes should be addressed.

markuswntr avatar Jun 23 '22 23:06 markuswntr

I think that this is contrary to the usual notions of argument for quaternions--in particular, the argument of a quaternion should be something like the imaginary part of it's logarithm, rather than a real angle. Is your use of argument precedented somewhere?

If it's not argument, what do we call this? I might just use halfAngle. Thoughts?

stephentyrone avatar Jun 30 '22 13:06 stephentyrone

That is unfortunate. I was not aware of this notion, thanks for clarifying. I had hoped it would solve ambiguity, not introduce a new one (that is arguably worse).

Is your use of argument precedented somewhere?

I first found the usage of the wording in Real Quaternionic Calculus Handbook. It denotes the quaternion "argument" as the angle θ, where θ is 2π-periodic, and the the "principle argument" as the angle in range [0,π], where the quadrant must otherwise be specified or understood. I did the initial tests with principleArgument but later switched to just calling it argument for brevity, and denoted it as "principle argument" in the header documentation.

If it's not argument, what do we call this? I might just use halfAngle. Thoughts?

I do not mind using halfAngle. It is intuitive, after all: it is "half the angle", which is correct. It might give halfAngle the attribute of being "dependent" on angle though, which it is not. Also, halfAngle is not that useful when dealing with Angle-Axis, just like axis is not for polar form, and so I had hoped that, with the new naming it would move both of them into their own "categories". I realize, both this cons are fairly weak, but I had hoped to solve them nonetheless :)

Edit: If we were to continue with halfAngle, I would still like to eliminate phase from the public API. In this case, the issue with source breaking changes would persist though.

markuswntr avatar Jul 01 '22 05:07 markuswntr

Ok, let's think about this a little more and see what makes sense.

stephentyrone avatar Jul 01 '22 11:07 stephentyrone

I reverted the changes to the naming of halfAngle, and updated the PR description to reflect the proposed API changes.

markuswntr avatar Oct 26 '22 20:10 markuswntr