Add `normalize` and `normalize!` #253

ranjanan · 2016-07-15T10:13:26Z

Add normalize and normalize!, which normalizes a vector according to the p-norm. Fixes #246 .

ranjanan · 2016-07-15T15:19:44Z

Failing on v0.3 :( I added docstrings to the functions, but there's no @doc defined for v0.3.

TotalVerb · 2016-07-16T02:41:42Z

@ranjanan I think the standard way to deal with docstrings is to use the syntax without @doc, that is:

"""
Docstring
"""
function x() end

This degrades to a no-op on v0.3, and will produce the correct docstring on v0.4.

ranjanan · 2016-07-16T02:43:18Z

Yes, I tried that first, but the docs won't show up at ?help. @yuyichao told me this was by design. That was why I shifted to using @doc.

TotalVerb · 2016-07-16T02:48:28Z

It is really no surprise that docs won't show up on v0.3, as docstrings were not implemented until v0.4. The @doc macro won't really help, as that was also implemented in v0.4.

The docs should show up on v0.4 without issue using the docstring syntax.

ranjanan · 2016-07-16T02:51:43Z

@TotalVerb I meant that on v0.4, the normal docstring syntax within an if block does not work (that is, show up at the REPL) by design. (But using @doc on v0.4 would make it show up at the REPL.) On v0.3 @doc wasn't part of Base Julia so it's not going to work. That is why, at @tkelman 's suggestion, I removed the docstrings and linked to it in the README.

TotalVerb · 2016-07-16T02:55:19Z

@ranjanan Ah, you are correct. I apologize for the misunderstanding.

tkelman · 2016-07-16T02:55:29Z

src/Compat.jl

@@ -1085,6 +1085,43 @@ if !isdefined(Base, :Threads)
    export Threads
 end

+if !isdefined(Base, :normalize)
+


don't put empty lines at the beginning or end of blocks like this

tkelman · 2016-07-16T03:00:46Z

needs rebasing

TotalVerb · 2016-07-16T21:00:31Z

You might need to provide copy_oftype too.

tkelman · 2016-07-16T21:02:26Z

test/runtests.jl

@@ -1286,3 +1286,12 @@ let
    @test norm(w) === 1.0
    @test norm(normalize!(v) - w, Inf) < eps()
 end
+
+# 0.5 style single argument `@boundscheck`


rebasing, not merging

ranjanan · 2016-07-18T05:07:39Z

@tkelman , yes, but I screwed up. I rebased on my local branch and my push got rejected, so I ended merging stuff from my remote branch on my local branch. I need to get better at this.

tkelman · 2016-07-18T05:13:45Z

when you rebase you need to force push over your branch

ranjanan · 2016-07-18T05:55:54Z

So the norm test is failing. Which is kinda weird, I copied this test from the Julia tests on master.

The same test passes on master but doesn't pass over here, even though I used the same implementations and same tests. @tkelman am I doing anything obviously wrong?

[ci skip]

Present in LinAlg module in v0.4 and v0.5 but not v0.3, so defined instead of importing

ranjanan · 2016-08-05T06:31:56Z

test/runtests.jl

+    δ = inv(prevfloat(typemax(Float64)))
+    v = [δ, -δ]
+
+    @test norm(v) === 7.866824069956793e-309


This is where the v0.3 test fails. norm(v) gives Inf

You know what, I'm not sure why I'm adding this test for norm. I'm testing normalize and normalize!

Well, actually it seems that norm itself behaves differently on v0.3 as compared to v0.4 and v0.5.

ranjanan · 2016-08-05T07:28:12Z

Okay, the function norm behaves differently in v0.3 as compared to v0.4 and v0.5. It seems to me that to support normalize and normalize!, we drop v0.3 support, or warn against flaky behaviour.

cc: @tkelman

stevengj · 2016-08-05T10:50:44Z

At some point the norm function's behavior was improved to reduce overflow/underflow problems. I don't think it's a big deal if Compat.normalize has the same limitations as norm in a given Julia version.

stevengj · 2016-08-05T12:33:15Z

test/runtests.jl

@@ -1301,9 +1301,13 @@ end
 let
    δ = inv(prevfloat(typemax(Float64)))
    v = [δ, -δ]
-
+    if VERSION > v"0.4.0-"


VERSION >= v"0.4.0-pre+7164", for JuliaLang/julia#1861

stevengj · 2016-08-05T12:34:47Z

test/runtests.jl

    w = normalize(v)
    @test w ≈ [1/√2, -1/√2]
-    @test norm(w) === 1.0
+    if VERSION > v"0.4.0-"
+        @test norm(w) === 1.0


This seems too strict. Should be @test norm(w) ≈ 1.0, and this should work on any version of Julia.

TotalVerb · 2016-08-08T13:10:07Z

test/runtests.jl

+    w = normalize(v)
+    @test w ≈ [1/√2, -1/√2]
+    @test_approx_eq norm(w) 1.0
+    @test norm(normalize!(v) - w, Inf) < eps()


It doesn't matter in this case, but ≈ is preferred to @test_approx_eq which is broken on some numeric types.

@TotalVerb You're right. I guess I'll use isapprox instead from now on. Would you like me to change it in this PR?

Better to use @test norm(w) ≈ 1.0

@TotalVerb @KristofferC it's weird, but the test fails on Julia v0.4 using isapprox but passes with @test_approx_eq

TotalVerb · 2016-08-09T02:33:07Z

@ranjanan The problem you are seeing on v0.4 is not caused by this change, merely exposed by it. It is rather a leaked rounding mode introduced by a different test. Here, try adding the following line to reset the rounding mode, so

    setrounding(T, RoundDown)
    @test rounding(T) == RoundDown

becomes

    setrounding(T, RoundDown)
    @test rounding(T) == RoundDown
    setrounding(T, RoundNearest)

TotalVerb · 2016-08-09T13:39:34Z

LGTM. Thanks for the contribution!

tkelman added the needs-tests label Jul 15, 2016

tkelman reviewed Jul 16, 2016
View reviewed changes

tkelman removed the needs-tests label Jul 16, 2016

tkelman reviewed Jul 16, 2016
View reviewed changes

ranjanan force-pushed the RA/norm branch from bd732de to 1ad6c21 Compare July 18, 2016 05:18

ranjanan added 5 commits July 19, 2016 11:53

Add normalize and normalize!

ce2b6ad

Remove docs and link to them in README

9da7193

[ci skip]

Add tests for normalize and normalize!

d11e9b5

Add normalize and normalize!

54ab513

Add copy_oftype

4565ae1

Present in LinAlg module in v0.4 and v0.5 but not v0.3, so defined instead of importing

ranjanan force-pushed the RA/norm branch from 543fd71 to 4565ae1 Compare July 19, 2016 06:25

@test -> @test_approx_eq

2577bcf

visr mentioned this pull request Aug 3, 2016

Base.normalize! on v0.4 JuliaGeo/LibGEOS.jl#19

Closed

ranjanan reviewed Aug 5, 2016
View reviewed changes

Remove norm test

a4759fe

Add VERSION checks on norm tests

0161634

stevengj reviewed Aug 5, 2016
View reviewed changes

Relax condition on a test

174f383

TotalVerb reviewed Aug 8, 2016
View reviewed changes

@test_approx_eq -> isapprox

43c6014

ranjanan added 2 commits August 9, 2016 10:13

Reset rounding mode

4d3ced9

Use right VERSION, fix tests

733748f

blakejohnson merged commit 8912b94 into JuliaLang:master Aug 11, 2016

ranjanan deleted the RA/norm branch August 11, 2016 03:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `normalize` and `normalize!` #253

Add `normalize` and `normalize!` #253

ranjanan commented Jul 15, 2016

ranjanan commented Jul 15, 2016

TotalVerb commented Jul 16, 2016

ranjanan commented Jul 16, 2016

TotalVerb commented Jul 16, 2016

ranjanan commented Jul 16, 2016 •

edited

TotalVerb commented Jul 16, 2016

tkelman Jul 16, 2016

tkelman commented Jul 16, 2016

TotalVerb commented Jul 16, 2016

tkelman Jul 16, 2016

ranjanan commented Jul 18, 2016

tkelman commented Jul 18, 2016

ranjanan commented Jul 18, 2016 •

edited

ranjanan Aug 5, 2016

ranjanan Aug 5, 2016

ranjanan Aug 5, 2016

ranjanan commented Aug 5, 2016 •

edited

stevengj commented Aug 5, 2016

stevengj Aug 5, 2016

stevengj Aug 5, 2016

TotalVerb Aug 8, 2016

ranjanan Aug 8, 2016

KristofferC Aug 8, 2016

ranjanan Aug 8, 2016

TotalVerb commented Aug 9, 2016

TotalVerb commented Aug 9, 2016

Add normalize and normalize! #253

Add normalize and normalize! #253

Conversation

ranjanan commented Jul 15, 2016

ranjanan commented Jul 15, 2016

TotalVerb commented Jul 16, 2016

ranjanan commented Jul 16, 2016

TotalVerb commented Jul 16, 2016

ranjanan commented Jul 16, 2016 • edited

TotalVerb commented Jul 16, 2016

Choose a reason for hiding this comment

tkelman commented Jul 16, 2016

TotalVerb commented Jul 16, 2016

Choose a reason for hiding this comment

ranjanan commented Jul 18, 2016

tkelman commented Jul 18, 2016

ranjanan commented Jul 18, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranjanan commented Aug 5, 2016 • edited

stevengj commented Aug 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TotalVerb commented Aug 9, 2016

TotalVerb commented Aug 9, 2016

Add `normalize` and `normalize!` #253

Add `normalize` and `normalize!` #253

ranjanan commented Jul 16, 2016 •

edited

ranjanan commented Jul 18, 2016 •

edited

ranjanan commented Aug 5, 2016 •

edited