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

[css-values] round(A,B) with B negative #4718

Closed
Loirooriol opened this issue Jan 30, 2020 · 6 comments
Closed

[css-values] round(A,B) with B negative #4718

Loirooriol opened this issue Jan 30, 2020 · 6 comments
Labels
css-values-4 Current Work

Comments

@Loirooriol
Copy link
Contributor

Loirooriol commented Jan 30, 2020

When rounding a value A with a negative precision B, I think there are two reasonable approaches:

  1. Ignore the sign, so that

    round(nearest, A, -B) = round(nearest, A, B)
    round(     up, A, -B) = round(     up, A, B)
    round(   down, A, -B) = round(   down, A, B)
    round(to-zero, A, -B) = round(to-zero, A, B)
    

    We find the two integer multiples of B closest to A, "lower B" and "upper B", with "lower B" < "upper B". And then we choose between by applying the strategy to "lower B" and "upper B":

    • For nearest, choose whichever of "lower B" and "upper B" that is closer to A. In case of tie, choose "upper B".
    • For up, choose "upper B".
    • For down, choose "lower B".
    • For to-zero, choose whichever of "lower B" and "upper B" that is closer to 0.

    In JS, for finite B,

    round(nearest, A, B) = Math.round(A/Math.abs(B)) * Math.abs(B)
    round(     up, A, B) = Math.ceil( A/Math.abs(B)) * Math.abs(B)
    round(   down, A, B) = Math.floor(A/Math.abs(B)) * Math.abs(B)
    round(to-zero, A, B) = Math.trunc(A/Math.abs(B)) * Math.abs(B)
    
  2. Use the sign to 'reverse' the strategy, so that

    round(nearest, A, -B) = -round(nearest, -A, B)
    round(     up, A, -B) = -round(     up, -A, B) = round(   down, A, B)
    round(   down, A, -B) = -round(   down, -A, B) = round(     up, A, B)
    round(to-zero, A, -B) = -round(to-zero, -A, B) = round(to-zero, A, B)
    

    We find the two integer multiples of B closest to A, B*n and B*(n+1). And then we choose between them by applying the strategy to n and n+1:

    • For nearest, choose whichever of B*n and B*(n+1) that is closer to A. In case of tie, choose B*(n+1).
    • For up, choose B*(n+1).
    • For down, choose B*n.
    • For to-zero, choose B*n if n is closer to 0 than n*1, else B*(n+1).

    In JS, for finite B,

    round(nearest, A, B) = Math.round(A/B) * B
    round(     up, A, B) = Math.ceil( A/B) * B
    round(   down, A, B) = Math.floor(A/B) * B
    round(to-zero, A, B) = Math.trunc(A/B) * B
    

Note there is no difference for to-zero. And for nearest it only matters in case of tie.

Currently the spec defines round() with (1). However, later it seems to assume (2):

mod() and rem() can also be defined directly in terms of other functions: mod(A, B) is equivalent to calc(A - round(down, A, B)), while rem(A, B) is equivalent to calc(A - round(to-zero, A, B)).

The former doesn't hold with (1), e.g. mod(3, -2) is supposed to be -1, but 3 - round(down, 3, -2) is 3 - 2 = 1. But it works with (2): 3 - 4 = -1. With (1), I guess the quote should say (A - round(down, A, B)) * sign(B)

So either keep the definition of round() as-is and fix the quote, or change the definition of round() and keep the quote. And add a note.

@Loirooriol Loirooriol added the css-values-4 Current Work label Jan 30, 2020
@tabatkins
Copy link
Member

The current definition of round() is correct. The quote is wrong; I tried to simplify the fuller expression of mod(A, B) = A - (B * floor(A/B)) into A - round(down, A, B), but as you note that's incorrect. I'll fix the note.

With (1), I guess the quote should say (A - round(down, A, B)) * sign(B)

No, changing the sign of a mod/rem result is never correct. Use a value other than 2/-2 in your tests and you'll see that immediately. ^_^

@Loirooriol
Copy link
Contributor Author

No, changing the sign of a mod/rem result is never correct

Sure, that's what happens when I write a long post in a hurry, something is gonna be wrong :)

I think the mostly correct one would be

A - round(down, A*sign(B), B)*sign(B)

e.g.

mod(4, 3) = 4 - round(down, 4, 3) = 4 - 3 = 1
mod(-4, 3) = -4 - round(down, -4, 3) = -4 - (-6) = 2
mod(-4, -3) = -4 + round(down, 4, 3) = -4 + 3 = -1
mod(4, -3) = 4 + round(down, -4, 3) = 4 - 6 = -2

the problem is with 0:

mod(+0, 3) = +0 - round(down, +0, 3) = +0 - 0 = +0 :)
mod(-0, 3) = -0 - round(down, -0, 3) = -0 + 0 = +0 :)
mod(+0, -3) != +0 + round(down, -0, 3) = +0 - 0 = +0 :(
mod(-0, -3) != -0 + round(down, +0, 3) = -0 + 0 = +0 :(

@tabatkins
Copy link
Member

I'm okay with that; addition isn't commutative with respect to -0 in IEEE semantics, so I'm not surprised it doesn't come out correctly from such an expression. I'll note that explicitly.

@Loirooriol
Copy link
Contributor Author

Maybe add a note clarifying that round(strategy, A, -B) = round(strategy, A, B)?

@Loirooriol
Copy link
Contributor Author

addition isn't commutative with respect to -0 in IEEE semantics

I'm not sure what you mean, it seems to me that -0 is the neutral element of addition.
Then, A + (-0) = A = (-0) + A, so it's commutative?
Maybe you meant that addition never produces -0?

@tabatkins
Copy link
Member

tabatkins commented Feb 3, 2020

Addition does produce -0, but only in -0 + -0; it does indeed act as the identity element there.

Subtracting is the real weird one, tho: only -0 - +0 produces -0; every other subtraction of zeros produces +0. Adding +0 to both sides to rearrange the equation, this implies that -0 = -0 + +0, but that's not true; that addition produces +0.

I think you're right that commutitivity isn't actually the law being broken there; I guess it's additive inverses don't work correctly around zeros (which is why flipping the sign like that isn't guaranteed to work right).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
css-values-4 Current Work
Projects
None yet
Development

No branches or pull requests

2 participants