Skip to content
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

Refine Hashable and Equatable on Quaternion #124

Merged
merged 13 commits into from
Jul 14, 2020
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 92 additions & 1 deletion Sources/QuaternionModule/Quaternion.swift
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@ import RealModule
/// Using the infinity norm avoids this problem entirely without significant
/// downsides. You can access the Euclidean norm using the `length` property.
/// See `Complex` type of the swift-numerics package for additional details.
///
/// Quaternions are frequently used to represent 3D transformations. It's
/// important to be aware that, when used this way, any quaternion and its
/// negation represent the same transformation, but they do not compare equal
/// using `==` because they are not the same quaternion.
/// You can compare quaternions as 3D transformations using `transformEquals()`.
public struct Quaternion<RealType> where RealType: Real & SIMDScalar {

/// The components of the 4-dimensional vector space of the quaternion.
Expand Down Expand Up @@ -248,6 +254,57 @@ extension Quaternion {
public var isPure: Bool {
real.isZero
}

/// A "canonical" representation of the value.
///
/// For normal quaternion instances with a RealType conforming to
/// BinaryFloatingPoint (the common case), the result is simply this value
/// unmodified. For zeros, the result has the representation (+0, +0, +0, +0).
/// For infinite values, the result has the representation (+inf, +0, +0, +0).
///
/// If the RealType admits non-canonical representations, the x, y, z and r
/// components are canonicalized in the result.
///
/// This is mainly useful for interoperation with other languages, where
/// you may want to reduce each equivalence class to a single representative
/// before passing across language boundaries, but it may also be useful
/// for some serialization tasks. It's also a useful implementation detail for
/// some primitive operations.
///
/// See also:
/// -
/// - `.canonicalizedTransform`
@_transparent
public var canonicalized: Self {
guard !isZero else { return .zero }
guard isFinite else { return .infinity }
return self.multiplied(by: 1)
}

/// A "canonical transformation" representation of the value.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to make this more precise. If I wasn't concerned about jargon, I would say "the canonical representative of the equivalence class of quaternions representing the same 3d transformation."

Copy link
Contributor Author

@markuswntr markuswntr Jun 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"The canonical representative of quaternions with non-negative real components representing the same 3D transformation."
Does that make sense? :)

I guess that's another task to think about over the weekend.

///
/// For normal quaternion instances with a RealType conforming to
/// BinaryFloatingPoint (the common case) and a non-negative real component,
/// the result is simply this value unmodified. For instances with a negative
/// real component, the result is a quaternion with a positive real component
/// of equal magnitude and an unmodified imaginary compontent (-r, x, y, z).
/// For zeros, the result has the representation (+0, +0, +0, +0). For
/// infinite values, the result has the representation (+inf, +0, +0, +0).
///
/// If the RealType admits non-canonical representations, the x, y, z and r
/// components are canonicalized in the result.
///
/// See also:
/// -
/// - `.canonicalized`
@_transparent
public var canonicalizedTransform: Self {
var canonical = canonicalized
if canonical.real.sign == .plus { return canonical }
// Clear the signbit of real even for -0
markuswntr marked this conversation as resolved.
Show resolved Hide resolved
canonical.real.negate()
return canonical
}
}

// MARK: - Additional Initializers
Expand Down Expand Up @@ -339,6 +396,17 @@ extension Quaternion where RealType: BinaryFloatingPoint {

// MARK: - Conformance to Hashable and Equatable
extension Quaternion: Hashable {
/// Returns a Boolean value indicating whether two values are equal.
///
/// Equality is the inverse of inequality. For any values *a* and *b*,
/// `a == b` implies that `a != b` is `false`.
markuswntr marked this conversation as resolved.
Show resolved Hide resolved
///
/// - Important:
/// Quaternions are frequently used to represent 3D transformations. It's
/// important to be aware that, when used this way, any quaternion and its
/// negation represent the same transformation, but they do not compare
/// equal using `==` because they are not the same quaternion. You can
/// compare quaternions as 3D transformations using `transformEquals()`.
@_transparent
public static func == (lhs: Quaternion, rhs: Quaternion) -> Bool {
// Identify all numbers with either component non-finite as a single "point at infinity".
Expand All @@ -350,6 +418,24 @@ extension Quaternion: Hashable {
return lhs.components == rhs.components
}

/// Transformation equality comparison
///
/// Returns a Boolean value indicating whether the 3D transformations of this
/// quaternion equals the 3D transformation of `other`.
///
/// - Parameter other: The value to compare.
/// - Returns: True if the transformation of this quaternion equals `other`.
@_transparent
public func transformEquals(_ other: Quaternion) -> Bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we feel about how this reads at a use site?

if q.transformEquals(p) { /* ... */ }

I don't love that "transform" may read as a verb if you don't know what you're looking at. What if we used something like this instead:

if q.equals(as3DTransform: p) { /* ... */ }

I'm not totally set on this either way, but let's give it some thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we feel about how this reads at a use site?

if q.transformEquals(p) { /* ... */ }

Yeah, you are right. This can easily be misread. We could maybe use transformationEquals to make it more explicit.

if q.equals(as3DTransform: p) { /* ... */ }

I think, I do prefer this spelling over transformEquals/transformationEquals. If we want to (somewhat) reuse existing naming patterns, we could utilize append(contentsOf:) and insert(contentsOf:at:) on Array and use:

if q.equals(transformationOf: p) { /* ... */ }

Though, I think it is a little less obvious and does not mention 3D explicitly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's think about this over the weekend and see if we can make a provisional decision on Monday.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any further thoughts on this? If not, let's go with equals(as3DTransform:) for now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps q.isSame3DTransform(as: p) or some such?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to equals(as3DTransform:) for now. I think, it reads a little nicer at the call site.

// Identify all numbers with either component non-finite as a single "point at infinity".
guard isFinite || other.isFinite else { return true }
// For finite numbers, equality is defined componentwise. Cases where only
// one of lhs or rhs is infinite fall through to here as well, but this
// expression correctly returns false for them so we don't need to handle
// them explicitly.
return components == other.components || components == -other.components
}

@_transparent
public func hash(into hasher: inout Hasher) {
// There are two equivalence classes to which we owe special attention:
Expand All @@ -358,8 +444,13 @@ extension Quaternion: Hashable {
// representation. The correct behavior for zero falls out for free from
// the hash behavior of floating-point, but we need to use a
// representative member for any non-finite values.
// For any normal values we use the "canonical transform" representation,
// where real is always non-negative. This allows people who are using
// quaternions as rotations to get the expected semantics out of collections
// (while unfortunately producing some collisions for people who are not,
// but not in too catastrophic of a fashion).
markuswntr marked this conversation as resolved.
Show resolved Hide resolved
if isFinite {
components.hash(into: &hasher)
canonicalizedTransform.components.hash(into: &hasher)
} else {
hasher.combine(RealType.infinity)
}
Expand Down
67 changes: 67 additions & 0 deletions Tests/QuaternionTests/PropertyTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -100,13 +100,80 @@ final class PropertyTests: XCTestCase {
XCTAssertEqual(infs[0], i)
XCTAssertEqual(infs[0].hashValue, i.hashValue)
}
// Validate that all *normal* values hash their absolute components, so
// that rotations in *R³* of `q` and `-q` will hash to same value.
let pairs: [(lhs: Quaternion<T>, rhs: Quaternion<T>)] = [
(
Quaternion<T>(real: .pi, imaginary: .pi, .pi, .pi),
Quaternion<T>(real: -.pi, imaginary: .pi, .pi, .pi)
markuswntr marked this conversation as resolved.
Show resolved Hide resolved
), (
Quaternion<T>(real: .pi, imaginary: -.pi, .pi, .pi),
Quaternion<T>(real: -.pi, imaginary: -.pi, .pi, .pi)
), (
Quaternion<T>(real: .pi, imaginary: .pi, -.pi, .pi),
Quaternion<T>(real: -.pi, imaginary: .pi, -.pi, .pi)
), (
Quaternion<T>(real: .pi, imaginary: .pi, .pi, -.pi),
Quaternion<T>(real: -.pi, imaginary: .pi, .pi, -.pi)
), (
Quaternion<T>(real: .pi, imaginary: -.pi, -.pi, .pi),
Quaternion<T>(real: -.pi, imaginary: -.pi, -.pi, .pi)
), (
Quaternion<T>(real: .pi, imaginary: -.pi, .pi, -.pi),
Quaternion<T>(real: -.pi, imaginary: -.pi, .pi, -.pi)
), (
Quaternion<T>(real: .pi, imaginary: .pi, -.pi, -.pi),
Quaternion<T>(real: -.pi, imaginary: .pi, -.pi, -.pi)
), (
Quaternion<T>(real: .pi, imaginary: -.pi, -.pi, -.pi),
Quaternion<T>(real: -.pi, imaginary: -.pi, -.pi, -.pi)
)
]
for pair in pairs {
XCTAssertEqual(pair.lhs.hashValue, pair.rhs.hashValue)
}
}

func testEquatableHashable() {
testEquatableHashable(Float32.self)
testEquatableHashable(Float64.self)
}

func testTransformationEquals<T: Real & SIMDScalar>(_ type: T.Type) {
let rotations: [(lhs: Quaternion<T>, rhs: Quaternion<T>)] = [
(
Quaternion<T>(real: -.pi, imaginary: -.pi, -.pi, -.pi),
Quaternion<T>(real: .pi, imaginary: .pi, .pi, .pi)
), (
Quaternion<T>(real: .ulpOfOne, imaginary: .ulpOfOne, .ulpOfOne, .ulpOfOne),
Quaternion<T>(real: -.ulpOfOne, imaginary: -.ulpOfOne, -.ulpOfOne, -.ulpOfOne)
), (
Quaternion<T>(real: .pi, imaginary: -.pi, .pi, -.pi),
Quaternion<T>(real: -.pi, imaginary: .pi, -.pi, .pi)
), (
Quaternion<T>(real: -.ulpOfOne, imaginary: -.ulpOfOne, .ulpOfOne, .ulpOfOne),
Quaternion<T>(real: .ulpOfOne, imaginary: .ulpOfOne, -.ulpOfOne, -.ulpOfOne)
),

// Zero and infinity must have equal rotations too
(
Quaternion<T>.zero,
-Quaternion<T>.zero
), (
-Quaternion<T>.infinity,
Quaternion<T>.infinity
),
]
for (lhs, rhs) in rotations {
XCTAssertTrue(lhs.transformEquals(rhs))
}
}

func testTransformationEquals() {
testTransformationEquals(Float32.self)
testTransformationEquals(Float64.self)
}

func testCodable<T: Real & SIMDScalar>(_ type: T.Type) throws {
let encoder = JSONEncoder()
encoder.nonConformingFloatEncodingStrategy = .convertToString(
Expand Down