Compute magnitudes in the "real part" of a type #278
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A `Magnitude` is _defined_ to be a _real_ number. But users can ask for
its value in, say, `std::complex<int>` (either explicitly or
implicitly). This is a problem, because in computing that value, we
check for overflow, which involves less-than comparisons... but complex
numbers are not ordered, so there is no meaningful sense of `<`!
To fix this, we introduce the notion of `RealPart<T>`, a type trait that
gives a type related to `T` that does have a notion of comparability.
(Since `Magnitude` is defined to be a real number, calling it "real
part" is a good fit.) `RealPart<T>` is just `T` in almost all cases,
but it becomes "the type of `T{}.real()`" whenever that exists. This
lets us support both `std::complex` and complex number types from other
libraries.
The core of the fix was to make sure all of our `Magnitude` operations
are taking place in `RealPart<T>` rather than `T`. This basically boils
down to the _call_ to `base_power_value`, and the _implementations_ for
`SafeCastingChecker`.
This change also has knock-on effects for implicit conversion policy.
It turns out the old implementation of `CoreImplicitConversionPolicy`
was always silently assuming that the type was already a real number
type. Therefore, we rename it by adding an `AssumingReal` suffix on the
end. The new `CoreImplicitConversionPolicy` passes `RealPart<Rep>` along
to this, after first checking for a new pitfall to guard against:
namely, implicitly converting a complex type to a real type.
clang struggles with complex division.
Some compilers can mix complex of float and int... some just can't.
geoffviola
approved these changes
Sep 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A
Magnitudeis defined to be a real number. But users can ask forits value in, say,
std::complex<int>(either explicitly orimplicitly). This is a problem, because in computing that value, we
check for overflow, which involves less-than comparisons... but complex
numbers are not ordered, so there is no meaningful sense of
<!To fix this, we introduce the notion of
RealPart<T>, a type trait thatgives a type related to
Tthat does have a notion of comparability.(Since
Magnitudeis defined to be a real number, calling it "realpart" is a good fit.)
RealPart<T>is justTin almost all cases,but it becomes "the type of
T{}.real()" whenever that exists. Thislets us support both
std::complexand complex number types from otherlibraries.
The core of the fix was to make sure all of our
Magnitudeoperationsare taking place in
RealPart<T>rather thanT. This basically boilsdown to the call to
base_power_value, and the implementations forSafeCastingChecker. We also use the real type throughout the:apply_magnitudetargets, for two reasons. First, there's an actualbug in clang related to complex-complex division. Second, it should be
faster at runtime to only divide by the real part.
This change also has knock-on effects for implicit conversion policy. It
turns out the old implementation of
CoreImplicitConversionPolicywasalways silently assuming that the type was already a real number type.
Therefore, we rename it by adding an
AssumingRealsuffix on the end.The new
CoreImplicitConversionPolicypassesRealPart<Rep>along tothis, after first checking for a new pitfall to guard against: namely,
implicitly converting a complex type to a real type.
Fixes #228.