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.

Sign up for GitHub

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

Jump to bottom

Add math.copySign #16406

Merged
merged 17 commits into from
Dec 30, 2020
Merged

Add math.copySign #16406

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
b77da8b
add math.copySign
ringabout Dec 20, 2020
44a3295
change
ringabout Dec 20, 2020
62a250f
go
ringabout Dec 25, 2020
cfbddfe
Merge remote-tracking branch 'upstream/devel' into add_copysign
ringabout Dec 25, 2020
7891b6c
done
ringabout Dec 26, 2020
cd1c38c
Merge remote-tracking branch 'upstream/devel' into add_copysign
ringabout Dec 28, 2020
ac90382
fix + tests
ringabout Dec 28, 2020
7b4994e
fix
ringabout Dec 28, 2020
2cafbc2
better tests
ringabout Dec 28, 2020
e1b4468
fix
ringabout Dec 28, 2020
cfbe626
fix
ringabout Dec 29, 2020
344a3e8
docs
ringabout Dec 29, 2020
0e6ac7d
reduce examples
ringabout Dec 29, 2020
0b845d8
add todo
ringabout Dec 29, 2020
a3beedf
more
ringabout Dec 29, 2020
7280070
better
ringabout Dec 29, 2020
f8d5208
fix
ringabout Dec 29, 2020
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
6 changes: 6 additions & 0 deletions compiler/vmops.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ from math import sqrt, ln, log10, log2, exp, round, arccos, arcsin,
arctan, arctan2, cos, cosh, hypot, sinh, sin, tan, tanh, pow, trunc,
floor, ceil, `mod`

when declared(math.copySign):
from math import copySign

from os import getEnv, existsEnv, dirExists, fileExists, putEnv, walkDir, getAppFilename
from md5 import getMD5
from sighashes import symBodyDigest
Expand Down Expand Up @@ -168,6 +171,9 @@ proc registerAdditionalOps*(c: PCtx) =
wrap1f_math(floor)
wrap1f_math(ceil)

when declared(copySign):
wrap2f_math(copySign)

wrap1s(getMD5, md5op)

proc `mod Wrapper`(a: VmArgs) {.nimcall.} =
Expand Down
43 changes: 20 additions & 23 deletions lib/pure/math.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,8 @@ when defined(c) or defined(cpp):
proc c_isnan(x: float): bool {.importc: "isnan", header: "<math.h>".}
# a generic like `x: SomeFloat` might work too if this is implemented via a C macro.

proc c_copysign(x, y: float32): float32 {.importc: "copysignf", header: "math.h".}
proc c_copysign(x, y: float64): float64 {.importc: "copysign", header: "math.h".}
proc c_copysign(x, y: cfloat): cfloat {.importc: "copysignf", header: "<math.h>".}
proc c_copysign(x, y: cdouble): cdouble {.importc: "copysign", header: "<math.h>".}

func binom*(n, k: int): int =
## Computes the `binomial coefficient <https://en.wikipedia.org/wiki/Binomial_coefficient>`_.
Expand Down Expand Up @@ -157,41 +157,38 @@ func isNaN*(x: SomeFloat): bool {.inline, since: (1,5,1).} =
else: result = c_isnan(x)

func copySign*[T: SomeFloat](x, y: T): T {.inline, since: (1, 5, 1).} =
ringabout marked this conversation as resolved.
Show resolved Hide resolved
## Returns a value with the magnitude of `x` and the sign of `y`.
## Returns a value with the magnitude of `x` and the sign of `y`;
## this works even if x or y are NaN or zero, both of which can carry a sign.
runnableExamples:
doAssert copySign(10.0, -1.0) == -10.0
doAssert copySign(-10.0, -1.0) == -10.0
doAssert copySign(-10.0, 1.0) == 10.0
doAssert copySign(10'f32, -1.0) == -10.0

doAssert copySign(Inf, -1.0) == -Inf
doAssert copySign(-Inf, 1.0) == Inf
doAssert copySign(1.0, -0.0) == -1.0
doAssert copySign(0.0, -0.0) == -0.0
doAssert copySign(-1.0, 0.0) == 1.0
doAssert copySign(10.0, 0.0) == 10.0

doAssert copySign(Inf, -1.0) == -Inf
doAssert copySign(-Inf, 1.0) == Inf
doAssert copySign(-1.0, NaN) == 1.0
timotheecour marked this conversation as resolved.
Show resolved Hide resolved
doAssert copySign(10.0, NaN) == 10.0

doAssert copySign(NaN, 0.0).isNaN
doAssert copySign(NaN, -0.0).isNaN

# fails in VM and JS backend
Copy link
Member

Choose a reason for hiding this comment

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

it won't fail in VM for a nim binary compiled after your PR

Copy link
Member Author

Choose a reason for hiding this comment

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

The same story as #16406 (comment)

doAssert copySign(1.0, copySign(NaN, -1.0)) == -1.0

# TODO use signbit for examples
template impl() =
ringabout marked this conversation as resolved.
Show resolved Hide resolved
if y.isNaN:
if y > 0.0 or (y == 0.0 and 1.0 / y > 0.0):
Copy link
Member

Choose a reason for hiding this comment

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

better code for js via Object.is, see #16505 (comment)

result = abs(x)
elif y <= 0.0:
result = -abs(x)
ringabout marked this conversation as resolved.
Show resolved Hide resolved
else: # must be NaN
result = abs(x)
else:
if y > 0.0 or (y == 0.0 and 1.0 / y > 0.0):
result = abs(x)
else:
result = -abs(x)

when nimvm:
impl()
when defined(js): impl()
else:
when not defined(js):
result = c_copysign(x, y)
else:
impl()

when nimvm: impl()
else: result = c_copysign(x, y)

func classify*(x: float): FloatClass =
## Classifies a floating point value.
Expand Down
12 changes: 9 additions & 3 deletions tests/stdlib/tmath.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,9 +338,7 @@ template main =
doAssert copySign(10.0, 0.0) == 10.0
doAssert copySign(-1.0, NaN) == 1.0
doAssert copySign(10.0, NaN) == 10.0
doAssert copySign(-1.0, -NaN) == 1.0
doAssert copySign(10.0, -NaN) == 10.0


doAssert copySign(NaN, NaN).isNaN
doAssert copySign(-NaN, NaN).isNaN
doAssert copySign(NaN, -NaN).isNaN
Expand All @@ -350,5 +348,13 @@ template main =
doAssert copySign(-NaN, 0.0).isNaN
doAssert copySign(-NaN, -0.0).isNaN

when nimvm:
discard
else:
when not defined(js):
doAssert copySign(-1.0, -NaN) == 1.0
Copy link
Member

Choose a reason for hiding this comment

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

until timotheecour#499 is fixed, this test is meaningless:

        doAssert copySign(-1.0, -NaN) == 1.0
        doAssert copySign(-1.0, NaN) == 1.0

both pass currently, which is not correct behavior IMO

IMO doAssert copySign(-1.0, NaN) == 1.0 is probably correct (we can check with signbit once we have it), but doAssert copySign(-1.0, -NaN) == 1.0 is probably not correct, and should be:

when false: # xxx bug
  doAssert copySign(-1.0, -NaN) == -1.0

likewise with doAssert copySign(10.0, -NaN) == 10.0

doAssert copySign(10.0, -NaN) == 10.0
doAssert copySign(1.0, copySign(NaN, -1.0)) == -1.0 # fails in VM
Copy link
Member

@timotheecour timotheecour Dec 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

this test actually works in VM so long nim is compiled after this PR.
Simplest is to defined nimHasCopySign in condsyms.nim and then:

    template fn() = doAssert copySign(1.0, copySign(NaN, -1.0)) == -1.0
    when defined(js): discard # xxx bug
    else:
      when defined(nimHasCopySign): fn()
      else:
        when nimvm: discard
        else: fn()

(refs timotheecour#498)

note, the same nimHasCopySign could be re-used in your other math PR's that need vmops to avoid creating too many condsyms symbols (until we finally reconsider the much better nimVersionCT approach, refs #14648)

Copy link
Member Author

Choose a reason for hiding this comment

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

I would rather make a follow-up PR to enable this test.

Copy link
Member Author

@ringabout ringabout Dec 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

And tmath.nim should support JS, I could try to enable JS tests(at least for copySign) in the next PR.

Copy link
Member

Choose a reason for hiding this comment

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

ya we definitely need this (in another PR as you say); refs timotheecour#494 (comment) ; it just requires a few disabling's for js to work (and vm already works)


static: main()
main()