SI-3235 math.round() returns wrong results for Int and Long

[Ichoran]

Minimal fix for SI-3235.

No deprecation or migration messages; if anyone used this to clip Long into Int (with rounding errors from Float), they will observe altered behavior.

Test verifies behavior.

[Ichoran]

@non @soc @adriaanm - This is as minimal of a fix as I can generate. It fixes the issue, nothing more or less. No rint, no propagation to all small-integer-like types, no deprecation warnings, just 123456789.round that is equal to what everyone would expect. (Plus the equivalent behavior for the equivalent problem with Long.)

[non]

👍

[soc]

No. No. No. No. No. I'm slowly losing my mind. What's wrong with not making things any worse?

[Ichoran]

@soc - I'm baffled. Why is this making things worse? 123456789.round is horrible, and this fixes it and nothing else (doesn't it?). What here is even worse than the behavior of 123456789.round?

[soc]

As mentioned for the umpteenth times, the return types are all over the place. Yes, it's kind of unfortunate that it is that way for Double#round and Float#round, but there is absolutely no excuse for repeating the same mistake again.

If you think this is the right approach, please, in god's name finally tell me what Complex#round is supposed to return if Complex is no option, or what BigInteger#round is supposed to return if BigInteger is no option in your opinion.

What here is even worse than the behavior of 123456789.round?

Yes. It's better to be broken in one way, than to be broken in different ways, depending on the Scala version. If you can't or don't want to fix it, just leave it as-is.

[Ichoran]

@soc - You're conflating two different issues. One: is there a more regular way to do this? Yes, there is. Unfortunately it breaks existing code and existing expectations. Two: can we protect users from pure nonsense? Yes. This is how. (Adding a deprecation warning would be even better. For reasons I don't understand, you seemed to object to that. It's less essential than the method that catches the problematic behavior, though, so I left it off this time.)

When we have time to redo the entire numeric hierarchy and/or redo whether primitives get promoted, we can revisit the issue. In the meantime, I don't think leaving unguarded pitfalls is doing anyone any favors. I don't think this is a time when regularity should trump correctness.

As to BigInt#round--you shouldn't be rounding your BigInts. If you are, you're confused. That should be a type error. Likewise with Complex, due to (1) the non-obvious choice of nearest real integer vs. nearest integer-valued lattice point (i.e. integer real and imaginary part), and (2) the lack of common utility in performing either transformation. That these things are tricky to get right is the reason to not hold up this fix waiting until everything is nailed down as well as it can be.

[adriaanm]

@soc it would help to understand why it's bad for the return types to be
"all over the place". Some might say they are "more precise". As discussed
earlier, if you later want to capture these operations in a type class, a
type member can be used to abstract over it (and be instantiated to the
right type for every instance of the type class).

On Tuesday, February 25, 2014, soc notifications@github.com wrote:

As mentioned for the umpteenth times, the return types are all over the
place. Yes, it's kind of unfortunate that it is that way for Double#roundand
Float#round, but there is absolutely no excuse for repeating the same
mistake again.

If you think this is the right approach, please, in god's name finally
tell me what Complex#round is supposed to return if Complex is no option,
or what BigInteger#round is supposed to return if BigInteger is no option
in your opinion.

Reply to this email directly or view it on GitHubhttps://github.com//pull/3581#issuecomment-36012620
.

[soc]

@Ichoran The approach I have proposed doesn't break "existing code and existing expectations" PLUS it's more regular PLUS it's protecting users from nonsense.

As to BigInt#round--you shouldn't be rounding your BigInts. If you are, you're confused. That should be a type error.

And guess who did a lot of work to prevent that from being a type error in the first place? Hint: It wasn't me. That we basically have to sprinkle round/ceil/floor on number types regardless of actual utility isn't the consequence of not knowing it better, it's the consequence of knowing better and acting against it.

Likewise with Complex ...

Despite the fact that somehow most languages/libraries know exactly what is meant by "rounding a complex", just pick a different random numeric type (which in your opinion needs a rounding operatiuon) and explain how you are going to fit the value of that type's round method into an Int or a Long.

@adriaanm How is <short>.round: Int more precise than <short>.round: Short?
How is <int>.floor: Double more precise than <int>.floor: Int?

The example with the typeclass was an example in which I tried to make it more obvious why it is important to be consistent. Consistency matters even if we didn't have that typeclass. And as mentioned above, adding an additional type member comes with additional complexity, still doesn't answer what the type member should be and what's the actual benefit of it.

instantiated to the right type for every instance of the type class

What's the supposed type member of such typeclass instances and where would they differ in a meaningful way from returning the same type itself?

[adriaanm]

Very roughly:

class Complex[T](real: T, imaginary: T) { type RoundedComponent ; def round: Comlex[RoundedComponent]  }
implicit object ComplexDouble(real: Double, imaginary: Double) extends Complex[Double] { type RoundedComponent = Long; def round = Complex(real.round, imaginary.round) }

Let's be realistic. We really can't be working on design the day the RC is cut. The debate was over which methods we were going to deprecate and which ones needed to be overridden to avoid the unexpected widenings during rounding.

[adriaanm]

@adriaanm How is .round: Int more precise than .round: Short?
How is .floor: Double more precise than .floor: Int?

What @Ichoran said: there's no reason round those.

[Ichoran]

@soc - Adding ceil and floor to primitive integer types clutters the interface for a benefit that you haven't yet described. Rather than "having to", I suggest: don't. There is no more need (in terms of types) to have to add those than to have to add toRadians. That rint isn't there, due to your distaste for the name, should break expectations. It is only round that has the problem due to it returning a type it can widen from; everything else only needs to be overloaded if one decides to try to brute force preventing widening. It's a losing battle, especially in the face of (2: Byte) + (2: Byte).

My fix is basically your fix, less the clutter of trunc and ceil. If this is now too minimal, and you think round should go on Byte and such as well, I suppose there's some argument for that. But I don't know why we're rounding these things anyway. This is why I just added a deprecation message before: don't round your Ints. You've probably made a mistake if you're trying. That seems the best way to grasp a little more sanity given the situation we're in.

Also: you already can't round BigInt, so I'm not sure what you're talking about there.

[adriaanm]

Finally, it seems desirable to get a long when rounding a double, and not a double (so you can't have the T => T signature for the round function, it must be ad-hoc). I'm not exactly sure, but I'd expect longs to be faster for many operations (say you're subtracting the rounding of two doubles).

[Ichoran]

@adriaanm - Due to implementation details, rint(x).toLong - rint(y).toLong is actually faster than round(x) - round(y). And a lot of the time you don't want a Long but an Int, so you write round(x).toInt, which might surprise you if the number is out of Int range but in Long range. The default round is a strange beast. I'm not sure I want to defend it too heavily.

[soc]

@adriaanm

Very roughly: [...]

Eh what ... and now we are back exactly at square one, converting values to types which can't represent those values?

The reason is called "consistency". Something we valued quite a lot in the past. Maybe our standards have changed and I didn't get the notice?

it seems desirable to get a long when rounding a double

Why? For all the excitement those invisible bugs bring to developers?

@Ichoran

But I don't know why we're rounding these things anyway.

I'm wondering what's more ironic: That you first fought tooth and nails against the principled solution of not converting unrelated, incompatible numeric types and are now complaining that the alternatives suck ... or that you don't even seem to realize that.

We would have never needed to add these methods if we just stopped converting numbers just for fun. But considering that we are stuck with it, you are complaining that having to play defense against those conversions is kind of inconvenient ... no shit Sherlock!?

I would have preferred a better solution, so don't blame the unfortunate consequences of your choices on me.

[adriaanm]

Please edit your comment for tone. We are having a technical discussion
here and I expect everyone to behave accordingly.

Please also consider the big picture. This is one small corner of the
language. We're not going to clean it up on the day of RC1. Best we can do
is some sweeping. Set your expectations accordingly.

On Tuesday, February 25, 2014, soc notifications@github.com wrote:

@adriaanm https://github.com/adriaanm

Very roughly: [...]

Eh what ... and now we are back exactly at square one, converting values
to types which can't represent those values?

The reason is called "consistency". Something we valued quite a lot in the
past. Maybe our standards have changed and I didn't get the notice?

it seems desirable to get a long when rounding a double

Why? For all the excitement those invisible bugs bring to developers?

@Ichoran https://github.com/Ichoran

But I don't know why we're rounding these things anyway.

I'm wondering what's more ironic: That you first fought tooth and nails
against the principled solution of not converting unrelated,
incompatible numeric types and are now complaining that the alternatives
suck ... or that you don't even seem to realize that.

We would have never needed to add these methods if we just stopped
converting numbers just for fun. But considering that we are stuck with it,
you are complaining that having to play defense against those conversions
is kind of inconvenient ... no shit Sherlock!?

I would have preferred a better solution, so don't blame the unfortunate
consequences of your choices on me.

Reply to this email directly or view it on GitHubhttps://github.com//pull/3581#issuecomment-36024583
.

[Ichoran]

New interpretation of minimal--minimal and maximally helpful for correctness.

@soc - Deprecation (or another way to generate a warning or error) can catch this single extra-problematic case for widening. This is the best minimal solution (for correctness), so I'm changing my PR to that. It will fix bugs and provide an alert about changed behavior. I apologize if it offends the sensibilities of some to use this (not very elegant) mechanism to catch widening in this case where it's really problematic.

@non - One more check, please?

@adriaanm - I doubt it's possible to do any better for code correctness with a simple fix (at least with the annotations I know of).

[Ichoran]

@soc - Incidentally, for x: Double, if abs(x) is more than 2^53 and you call round(x), you're already probably hosed. Not fitting into a Long is hardly the worst of it.

[soc]

@adriaanm

Please also consider the big picture. This is one small corner of the language. We're not going to clean it up on the day of RC1. Best we can do is some sweeping. Set your expectations accordingly.

That's exactly what I'm doing. Not making things worse than they currently are, so that we can have a realistic change for a better fix in the future instead of piling hacks upon hacks.
I'm already seeing who will complain about "bike-shedding" in the future by pointing out that "we have already changed round in 2.11".

Anyway, things work as they are supposed to in my branch without requiring any of these hacks here, and that's all what I'm going to care about in the future.

[adriaanm]

Ok, thanks for your input and your patience. I'll consider this PR good to go then, assuming @non agrees.

[non]

👍

[adriaanm]

Great! One small step for scalac,....

[som-snytt]

Future { bikeShed } foreach (_.paint()).

[Ichoran]

@som-snytt - But you missed the most interesting (devious?) part:

package assumptions {
  implicit val bestColor = Color.Green
  implicit class Painter(b: BikeShed) {
    def paint()(implicit color: Color): Unit = { b.color = color }
  }
}
import assumptions._
Future { bikeShed } foreach (_.paint())