isapprox with nans keyword (issue 19936) by JeffreySarnoff · Pull Request #20022 · JuliaLang/julia

JeffreySarnoff · 2017-01-13T21:35:29Z

No description provided.

StefanKarpinski

Maybe add a test for isapprox over arrays containing NaNs too?

StefanKarpinski · 2017-01-13T21:48:55Z

test/floatfuncs.jl

+for nan in (Float16,Float32,Float64)
+    nan = elty(NaN)
+    @test isapprox(nan, nan) == false
+    @test isapprox(nan, nan, nans=true) == true7


typo:true7?

StefanKarpinski · 2017-01-13T22:36:53Z

test/floatfuncs.jl

+
+# isapprox
+for nan in (Float16,Float32,Float64)
+    nan  = elty(NaN)


I don't think these tests mean what you want them to mean anymore.

tkelman · 2017-01-13T22:59:06Z

base/floatfuncs.jl

 Inexact equality comparison: `true` if `norm(x-y) <= atol + rtol*max(norm(x), norm(y))`. The
-default `atol` is zero and the default `rtol` depends on the types of `x` and `y`.
+default `atol` is zero and the default `rtol` depends on the types of `x` and `y`. The default
+`nans` is false, meaning `isapprox(NaN,NaN) == false`; call with `nans=true` to have `isapprox(NaN,NaN) == true`.


describe what the keyword argument does before saying what its default is?

How about "Keyword argument nans (defaults to false) determines whether or not NaN values are considered equal to one another." ?

tkelman · 2017-01-13T23:00:04Z

test/floatfuncs.jl

+for elty in (Float16,Float32,Float64)
+    nan  = elty(NaN)
+    half = elty(0.5)
+    @test isapprox(nan, nan) == false


I'd just @test !isapprox(nan, nan) instead of comparing to true/false

StefanKarpinski · 2017-01-13T23:10:24Z

There's some trailing whitespace added here, otherwise it looks good!

StefanKarpinski · 2017-01-13T23:37:26Z

I would not add a new test file unless there are a lot of tests. You've also forgotten to git add the test file.

JeffreySarnoff · 2017-01-13T23:41:27Z

That test file already existed -- I just found it https://github.com/JeffreySarnoff/julia/edit/js/isapprox/test/floatapprox.jl

…

On Fri, Jan 13, 2017 at 6:37 PM, Stefan Karpinski ***@***.***> wrote: I would not add a new test file unless there are a lot of tests. You've also forgotten to git add the test file. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#20022 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABmqxnCd01QGiFgD-Vum9Sathre2RD6Oks5rSAq_gaJpZM4LjVXb> .

StefanKarpinski · 2017-01-13T23:44:38Z

Ah, ok. I see you added the tests back now. Carry on.

tkelman · 2017-01-14T02:39:01Z

base/floatfuncs.jl

-function isapprox(x::Number, y::Number; rtol::Real=rtoldefault(x,y), atol::Real=0)
-    x == y || (isfinite(x) && isfinite(y) && abs(x-y) <= atol + rtol*max(abs(x), abs(y)))
+function isapprox(x::Number, y::Number; rtol::Real=rtoldefault(x,y), atol::Real=0, nans::Bool=false)
+    x == y || (isfinite(x) && isfinite(y) && abs(x-y) <= atol + rtol*max(abs(x), abs(y))) || (nans && (isnan(x) & isnan(y)))


are there cases where it's faster to not short-circuit?

I don't think this is a particularly performance-critical operation.

almost certainly isn't, may as well be consistent with the other comparisons in this condition

what do you suggest doing?

StefanKarpinski · 2017-01-14T03:00:29Z

Introduces trailing white space, which is a failure.

JeffreySarnoff · 2017-01-14T03:05:16Z

do you mean (nans && isnan(x) && isnan(y))?

…

On Fri, Jan 13, 2017 at 10:03 PM, Tony Kelman ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In base/floatfuncs.jl <#20022>: > @@ -188,8 +189,8 @@ approximately equal component-wise. The binary operator `≈` is equivalent to `isapprox` with the default arguments, and `x ≉ y` is equivalent to `!isapprox(x,y)`. """ -function isapprox(x::Number, y::Number; rtol::Real=rtoldefault(x,y), atol::Real=0) - x == y || (isfinite(x) && isfinite(y) && abs(x-y) <= atol + rtol*max(abs(x), abs(y))) +function isapprox(x::Number, y::Number; rtol::Real=rtoldefault(x,y), atol::Real=0, nans::Bool=false) + x == y || (isfinite(x) && isfinite(y) && abs(x-y) <= atol + rtol*max(abs(x), abs(y))) || (nans && (isnan(x) & isnan(y))) almost certainly isn't, may as well be consistent with the other comparisons in this condition — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#20022>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABmqxmfJ8oOg-ErmQkhBCn9PrBprsg1Hks5rSDrzgaJpZM4LjVXb> .

tkelman · 2017-01-14T03:20:18Z

base/linalg/uniformscaling.jl

+function isapprox{T<:Number,S<:Number}(J1::UniformScaling{T}, J2::UniformScaling{S};
+                              rtol::Real=Base.rtoldefault(T,S), atol::Real=0, nans::Bool=false)
+    isapprox(J1.λ, J2.λ, rtol=rtol, atol=atol, nans=nans)
+end  


there's trailing whitespace here (you can find these with make check-whitespace locally), otherwise lgtm

That should do it .

tkelman · 2017-01-14T03:20:58Z

test/floatapprox.jl

 @test [0,Inf] ≈ [0,Inf]
 @test [0,Inf] ≉ [0,-Inf]
+
+# issue #19936 


also trailing whitespace here

tkelman · 2017-01-14T03:22:55Z

test/floatapprox.jl

 @test [0,Inf] ≈ [0,Inf]
 @test [0,Inf] ≉ [0,-Inf]
+
+# issue #19936 


also trailing whitespace here

tkelman

should be squashed on merge

JeffreySarnoff added 2 commits January 13, 2017 15:48

add nans keyword to isapprox

fe0866f

test isapprox with nans

bc14714

StefanKarpinski requested changes Jan 13, 2017

View reviewed changes

JeffreySarnoff added 4 commits January 13, 2017 16:55

fix typo

b8d1ae0

isapprox with nans keyword

734344d

isapprox with nans keyword

93f91b2

add tests for isapprox([nan],[nan])

d745d4b

StefanKarpinski reviewed Jan 13, 2017

View reviewed changes

fix bad name

05542af

tkelman reviewed Jan 13, 2017

View reviewed changes

StefanKarpinski approved these changes Jan 13, 2017

View reviewed changes

move isapprox tests into floatapprox.jl

67b8def

issue #19936

d9611df

tkelman reviewed Jan 14, 2017

View reviewed changes

fixes trailing whitespace?

ed1a291

JeffreySarnoff added 3 commits January 13, 2017 22:09

quash whitespace error

78796c8

isapprox description readability

21c6cba

change isapprox nan test short-circuiting

5fb491b

tkelman reviewed Jan 14, 2017

View reviewed changes

JeffreySarnoff added 2 commits January 13, 2017 22:23

remove trailing whitespace

cfe5db6

vanquish trailing whitespace

6b28ca8

tkelman approved these changes Jan 14, 2017

View reviewed changes

JeffreySarnoff mentioned this pull request Jan 14, 2017

Optionally treat NaNs as equal to each other in isapprox #19936

Closed

tkelman merged commit 2680a01 into JuliaLang:master Jan 14, 2017

JeffreySarnoff deleted the js/isapprox branch January 14, 2017 21:18

Uh oh!

Conversation

JeffreySarnoff commented Jan 13, 2017

Uh oh!

StefanKarpinski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StefanKarpinski commented Jan 13, 2017

Uh oh!

StefanKarpinski commented Jan 13, 2017

Uh oh!

JeffreySarnoff commented Jan 13, 2017 via email

Uh oh!

StefanKarpinski commented Jan 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StefanKarpinski commented Jan 14, 2017

Uh oh!

JeffreySarnoff commented Jan 14, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkelman left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants