[swift-evolution] [Review] SE-0067: Enhanced Floating Point Protocols

[Stephen Canon]

Comments inline.

> On Apr 20, 2016, at 12:04 PM, Xiaodi Wu <xiaodi.wu at gmail.com> wrote:
> 
> On Wed, Apr 20, 2016 at 5:38 AM, Stephen Canon <scanon at apple.com> wrote:
>> Hi Xiaodi —
>> 
>> Thanks for the detailed comments.  Some thoughts inline.
>> 
>>> On Apr 19, 2016, at 6:34 PM, Xiaodi Wu via swift-evolution <swift-evolution at swift.org> wrote:
>>> 
>>> * What is your evaluation of the proposal?
>>> 
>>> +1 in intent. Specifics require further refinement. For example:
>>> 
>>> Internal inconsistencies in capitalization:
>>> * `signalingNaN` but `isSignalingNan` and `isNan`
>> 
>> This is a typo.  Should be signalingNan.
>> 
>>> Parameter labels, or whatever they're called now, do not reflect newly
>>> adopted Swift syntax in SE-0046:
>>> * `static func maximum(x: Self, _ y: Self) -> Self` should be `static
>>> func maximum(_ x: Self, _ y: Self) -> Self`, etc.
>>> 
>>> Infelicitous use of prepositions to conform superficially to new
>>> naming guidelines:
>>> * `isEqual(to:)` is fine, but for consistency there's
>>> `isLessThanOrEqual(to:)`, which is not fine, because the preposition
>>> "to" applies only to "equal" and not to "less than”
>>> 
>>> Since `adding(_:)` is instead now an operator in the current version
>>> of the proposal, could comparison functions also be operators only?
>> 
>> They could, but you still need isUnordered(with: ) and isTotallyOrdered(with: ), as they don’t have operator equivalents.
> 
> My two cents are that the comparison functions that have operators
> should stick with those operators only instead of the names.
> Stylistically, I think `isLessThanOrEqual(to:)` is less than
> cromulent.
> 
> Some thoughts on `isUnordered(with:)` and `isTotallyOrdered(with:)`--
> 
> 1. As you mention below, implementations are free to expose
> IEEE754-mandated operations with names that differ from the
> specification. Looking to Java and C# as examples of how other C-style
> languages implement floating point types, C-style languages have taken
> a free hand in how they deal with comparison with NaN.
> 
> So long as we're not going down the road of having methods that expose
> distinct unordered-quiet and unordered-signaling comparison predicates
> as defined in IEEE754 (do any C-style languages do that?), there's
> probably some freedom to determine whether `isUnordered(with:)` is
> necessary as a standalone method or whether it's already sufficiently
> provided for by other methods. In the end, `x.isUnordered(with: y)` is
> really equivalent to `x.isNan || y.isNan`, which can be immediately
> grokked by any reader of code.
> 
> 2. `isTotallyOrdered(with:)` isn't quite apt as a name. As I
> understand it, totalOrder(x, y) "reflects" a total ordering such that,
> when x is ordered "below" y, the return value is true. (Both quoted
> words reflect the terminology used in the specification.) So perhaps
> `isTotallyOrdered(below:)` or even just `isOrdered(below:)` would be
> more apt. Certainly `with` is not the correct label for the parameter,
> as any two values that don't compare unordered should be "totally
> ordered with" each other. This is especially problematic if we're
> going to keep `isUnordered(with:)`, since those two methods would
> suggest a complementary logical relationship that just isn't there.

Yes, if we drop isUnordered(with:), then renaming this isOrdered(below:) would work.  I’m OK with this approach.

>>> Incorrect nomenclature in an attempt to correct previously misleading
>>> nomenclature:
>>> * `leastMagnitude` should be `leastPositive` or `minPositive`, because
>>> magnitudes can be zero: it's bonkers that
>>> `Double.minimumMagnitude(0.0, Double.leastMagnitude) <
>>> Double.leastMagnitude`!
>>> (Likewise, `leastNormalMagnitude` should be `leastNormalPositive` or
>>> `minPositive`, and `greatestFiniteMagnitude` should be
>>> `greatestFinite` or `maxFinite`)
>>> 
>>> Inconsistencies with Integer protocols (at least, as they are currently):
>>> * properties named "least..." or "greatest..." are inconsistent with
>>> conceptually similar properties such as `Int.min` and `Int.max`
>> 
>> `min` and `max` were deliberately avoided based on a discussion with the Apple standard library team; these properties don’t really behave like the integer bounds properties, so naming them similarly may be confusing.
> 
> I agree absolutely that `min` and `max`, standalone, are misleading
> and shouldn't be used. That said, I would argue that `maxFinite` is
> demonstrably the correct name because it's the most succinct one
> that's both accurate and consistent with other usage. Adding
> "magnitude" clarifies nothing here and in fact made me do a
> double-take on first reading: I had to think for a split second
> whether there exists a greater floating point value that's a finite
> non-magnitude: it's of course absurd, but you have to think about it
> the first time. Meanwhile, `greatestFinite` means exactly the same
> thing as `maxFinite`, but now you're introducing a different word
> where it's not necessary to clarify the meaning or differentiate from
> standalone `max`.

My biggest concern with `maxFinite` is that it would seem to also require a `minFinite`, which would then be easily confused with e.g. DBL_MIN (which means something radically different), and also implies that they might not be symmetric after all.  To my ear, “magnitude” more readily suggests the existing symmetry: namely that I can get the finite bounds as +/-.greatestFiniteMagnitude.

FWIW, 754 actually explicitly describes least[Positive]Magnitude as “the positive number of least magnitude” (the definition of nextUp, 5.3.1).

>> Your point about magnitudes being non-zero is reasonable, but I think you’ve taken it a step to far; it could be corrected by simply changing `leastMagnitude` to either `leastPositiveMagnitude` or `leastNonzeroMagnitude`.
> 
> By the same reasoning here, `minPositive` (instead of
> `leastMagnitude`) is equally accurate, more consistent with usage
> elsewhere, and probably more accessible to non-native English
> speakers. Throughout the IEEE specification, "magnitude" is used in
> relation to absolute values, which is not really in play here.
> 
> In any case, we agree that `leastMagnitude` must at minimum be renamed
> to exclude zero.
> 
>> `leastNormalMagnitude` and `greatestFiniteMagnitude` are accurate as is.  (`minPositive`, on the other hand, would be exceedingly misleading).  Can you expand on why you want to change them?  It seems like you simply prefer “positive” to “magnitude”?
> 
> I made a typo here (I blame jetlag): `minPositiveNormal` was what I
> meant to type. Again, my rationale is that it is the least deviation
> from `min` that accurately describes what's going on using the
> simplest words. Here, the distinction between normal and subnormal is
> unavoidable, but we don't need to talk about magnitudes, and there
> really is no difference between "min" and "least”.

One is a word, the other is an abbreviation.  The swift guidelines, for better or worse, counsel us to “avoid abbreviations”.  The fact that “min" is a term of art makes it plausible, but I don’t think we’re desperate to save two characters in the name.

> Also potentially useful (actually, definitely useful in implementing
> floating point strides) would be the properties `maxExactInteger`
> (and, I suppose, a corresponding `minExactInteger`).

An early draft of the protocol had these.  Naming this property is *hard*, because every floating-point value larger than `maxExactInteger` is … an exact integer.  If you want to be unambiguously precise, you end up with something horrible.  Ultimately I punted on this issue, but I would definitely support adding it in the future if an appropriate name can be found, or if a compelling use case arises (I don’t think it’s actually needed for implementing strides).

>>> Use of one term-of-art (`ulp`), as Swift naming guidelines allow and
>>> even encourage, but failure to use more widely understood terms of
>>> art:
>>> * `squareRoot()` should be `sqrt()`
>>> * something really ought to be done about
>>> `truncatingRemainder(dividingBy:)`--the fact that the comments tell
>>> the reader that `truncatingRemainder(dividingBy:)` is equivalent to C
>>> `fmod` suggests to me that `fmod` may be a widely understood
>>> term-of-art
>>> I argue strongly that Swift's naming guidelines about terms-of-art
>>> should be understood to encourage terms used widely in other languages
>>> for basic mathematical functions instead of written-out English
>>> equivalents--e.g. `asinh` instead of`inverseHyperbolicSine`. Looking
>>> to other C-style languages, all seem to accept these shorter terms
>>> as-is without writing them out.
>> 
>> sqrt( ) I could support, but fmod( ) is an absolutely terrible name for a relatively rarely-used function.  If there were a good term-of-art, I would want to use it, but AFAIK there isn’t.
>> 
>> I should note that the free functions sqrt( ) and fmod( ) won’t go away with this proposal.  They will continue to be supplied by the math overlay for Float, Double, CGFloat, just not for the FloatingPoint protocol.  So why do we need them in the FloatingPoint protocol at all?
>> 
>> The squareRoot( ) and remainder( ) methods are distinct from most of the other <math.h> functions in that IEEE 754 considers them to be "basic operations” as defined by clause 5 of the standard (IEEE 754 spells out the name “squareRoot” FWIW, though there’s no requirement that we follow that).
> 
> Even IEEE754 is interestingly inconsistent here. Throughout, it's
> written as "squareRoot", but when it comes to "recommended" functions,
> the reciprocal of the square root is written "rSqrt" (see table 9.1).
> I'd highly recommend setting a good example of when things are
> terms-of-art by going with "sqrt”.

Clause 9, being non-normative, didn't receive nearly as much editorial attention.

> While we're on the topic of IEEE754 section 5 functions, we're missing
> abs(x) in the protocol.

abs() would have come from SignedArithmetic, but you’re right that it’s now missing and should be added to FloatingPoint.

> And speaking of absolute value functions, IEEE754 calls it "minNumMag"
> and "maxNumMag" when comparing two values but, when it comes to
> summing, recommends a function called "sumAbs". A missed opportunity
> for consistency, IMO. When implementing in Swift, then, was there a
> rationale for having `minimumMagnitude` instead of the much shorter
> but equally grokkable `minAbs` (abs being, IMO, a term-of-art like
> sqrt)?

maxAbs is unclear (to me);  I could easily imagine that it returns max(abs(x), abs(y)).  Given that these methods have no precedent in C-family languages, there isn’t an actual term of art to follow, and the meaning of maxAbs isn’t particularly obvious.

> And then speaking of implementations of IEEE minNum and maxNum--are
> those static methods necessary in the protocol? What is gained over
> having just the comparison operators?

While one can write these operations in terms of the comparison operators, it’s a bit unwieldy to do so, and requires some heroics from the optimizer to turn such an expanded definition into e.g. the arm64 FMAXNM instruction or the AVX-512 VRANGE instruction.  It’s somewhat easier to wire up those optimizations this way.

Similarly, having these methods directly available is a useful optimization hook for user-defined types that conform to the protocol (though the protocol *will* provide default implementations, so you’re not obligated to do so).

>> Because of this it makes sense to require them as methods in the FloatingPoint protocol, rather than only as free functions.  [truncatingRemainder( ) is not required by IEEE 754, but it doesn’t impose a significant implementation burden and eases the transition for folks currently using operator %.]
> 
> I'm not sure I buy the "ease" argument. Surely, if folks are using
> operator %, it's equivalently difficult to transition either to the
> free function or to `truncatingRemainder()`? Suppose you deprecate %
> and recommend fmod(), then just leave `truncatingRemainder()` out of
> the protocol. After all, the rationale for deprecation is that people
> aren't using it correctly...

fmod() wraps the C stdlib fmod(), so only exists for types supported by the C stdlib.  The truncatingRemainder() method is available for all FloatingPoint types (these two sets of types are equivalent for Swift today, but if someone wants to write a library type that conforms to FloatingPoint, it would provide truncatingRemainder).