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

norm methods for AbstractSeries #128

Merged

lbenet merged 25 commits into JuliaDiff:master from blas-ko:norm

Oct 6, 2017

Contributor

blas-ko commented Sep 29, 2017

Follows discussion #127. isapprox is not yet added @PerezHz, but it may be easily included if we all agree with the norm methods here.

blas-ko added 2 commits

September 29, 2017 09:56


          Added norm methods to get a non-negative Number from an AbstractSeries.

ac693ce

Suggestions from @lbenet and @PerezHz


          Added some tests for norm.

b5fd520

lbenet reviewed

View reviewed changes

Member

lbenet left a comment

LGTM; I have included two suggestions that are related to the tests.

I am not sure if this implementation is enough to address this comment, without adding new methods for isapprox. Can you please check it?

test/mixtures.jl Outdated

@@ @@ -133,4 +133,10 @@ using Base.Test @@
                   @test xx*δx + Taylor1(typeof(δx),5) == δx + δx^2 + Taylor1(typeof(δx),5)
                   @test xx/(1+δx) == one(xx)
                   @test typeof(xx+δx) == Taylor1{TaylorN{Float64}}

Member

lbenet Sep 29, 2017

Maybe you should convert X and/or Y to Taylor1{TaylorN{Float64}}s and include another test for that.

Contributor Author

blas-ko Sep 29, 2017

Great, I'll do this in a moment!

test/onevariable.jl

@@ @@ -263,6 +263,17 @@ using Base.Test @@
                   @test string(ta(0)^3-3) == " - 3 + 1 t³ + 𝒪(t¹⁶)"
                   @test TaylorSeries.pretty_print(ta(3im)) == " ( 3 im )  + ( 1 ) t + 𝒪(t¹⁶)"
                   @test string(Taylor1([1,2,3,4,5], 2)) == string(Taylor1([1,2,3]))

Member

lbenet Sep 29, 2017

My suggestion here it to test the norm of something like t_C = complex(3.0,4.0) * t_a against the norm of complex(3.0,4.0) * a; as defined below, t_C is only a constant Taylor series.

Contributor Author

blas-ko Sep 29, 2017

I'll change this too!

lbenet mentioned this pull request

Adding specific norm methods for polynomials #127

Closed

coveralls commented Sep 29, 2017

Coverage increased (+0.01%) to 94.193% when pulling b5fd520 on blas-ko:norm into 347fce1 on JuliaDiff:master.

4 similar comments

coveralls commented Sep 29, 2017

Coverage increased (+0.01%) to 94.193% when pulling b5fd520 on blas-ko:norm into 347fce1 on JuliaDiff:master.

coveralls commented Sep 29, 2017

Coverage increased (+0.01%) to 94.193% when pulling b5fd520 on blas-ko:norm into 347fce1 on JuliaDiff:master.

coveralls commented Sep 29, 2017

Coverage increased (+0.01%) to 94.193% when pulling b5fd520 on blas-ko:norm into 347fce1 on JuliaDiff:master.

coveralls commented Sep 29, 2017 •

edited

Loading

Coverage increased (+0.01%) to 94.193% when pulling b5fd520 on blas-ko:norm into 347fce1 on JuliaDiff:master.

Member

lbenet commented Sep 29, 2017

Do you understand why some tests fail in travis and others pass?

Incidentally, at some point you will have to rebase to current master.

Member

lbenet commented Sep 29, 2017 •

edited

Loading

I am restarting the tests; it may be some glitch...

Contributor Author

blas-ko commented Sep 29, 2017

I am not sure if this implementation is enough to address this comment, without adding new methods for isapprox. Can you please check it?

Methods have to be extended for AbstractSeries as isapprox is defined specifically for each Number subtype. The way @PerezHz did it seems as the correct way to go.

Do you understand why some tests fail in travis and others pass?
No idea, honestly...

Incidentally, at some point you will have to rebase to current master.

Yes, I'll do your proposed changes and a rebase!

Member

lbenet commented Sep 29, 2017

julia> methods(isapprox)
# 3 methods for generic function "isapprox":
isapprox(x::Number, y::Number; rtol, atol, nans) in Base at floatfuncs.jl:204
isapprox(x::AbstractArray, y::AbstractArray; rtol, atol, nans, norm) in Base.LinAlg at linalg/generic.jl:1280
isapprox(J1::UniformScaling{T}, J2::UniformScaling{S}; rtol, atol, nans) where {T<:Number, S<:Number} in Base.LinAlg at linalg/uniformscaling.jl:178

The reason I think we may get it for free (using the implementation of norm) is because of the first method above.

Member

lbenet commented Sep 30, 2017

Tests are passing! It was a travis thing...

Contributor

PerezHz commented Sep 30, 2017

The reason I think we may get it for free (using the implementation of norm) is because of the first method above.

Julia 0.6 code for the relevant isapprox method that you mention reads (JuliaLang/julia/src/floatfuncs.jl, line 204):

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))
end

Note the use of the abs function in the expression abs(x-y) <= atol + rtol*max(abs(x), abs(y))) above.

Using this method, if on this branch (blas-ko:norm) we do

using TaylorSeries

import Base: rtoldefault, isfinite

# rtoldefault ans isfinite overloading for TaylorSeries
rtoldefault(::Type{T}) where T<:AbstractSeries = sqrt(eps())
isfinite(x::AbstractSeries) = !isinf(x)

t = Taylor1(25)
p = sin(t)
q = sin(t+eps())

then p ≈ p returns true, q ≈ q returns true, but q ≈ p returns the following error:

ERROR: ArgumentError: The 0th order Taylor1 coefficient must be non-zero
(abs(x) is not differentiable at x=0).
Stacktrace:
 [1] abs(::TaylorSeries.Taylor1{Float64}) at /Users/Jorge/fork-bo/TaylorSeries.jl/src/other_functions.jl:86
 [2] #isapprox#399(::Float64, ::Int64, ::Bool, ::Function, ::TaylorSeries.Taylor1{Float64}, ::TaylorSeries.Taylor1{Float64}) at ./floatfuncs.jl:204
 [3] isapprox(::TaylorSeries.Taylor1{Float64}, ::TaylorSeries.Taylor1{Float64}) at ./floatfuncs.jl:204

Which is related to the fact that abs is not defined at zero for Taylor1 polynomials. So there are two solutions that I can think of:

overloading isapprox for TaylorSeries polynomials
re-defining abs in TaylorSeries

On another comment, I proposed an overloading of isapprox for TaylorSeries polynomials, which I print here for reference:

function isapprox{T<:AbstractSeries}(x::T, y::T; rtol::Real=rtoldefault(x,y), atol::Real=0, nans::Bool=false)
    x == y || (isfinite(x) && isfinite(y) && norm(x-y,1) <= atol + rtol*max(norm(x,1), norm(y,1))) || (nans && isnan(x) && isnan(y))
end

Note that this is essentially the same method as Base's, except for the use of norm(...,1) instead of abs(...). The reasoning behind this is that, Base.isapprox relies on abs returning a <:Real value. Further, in order for things to work properly, abs for TaylorSeries polynomials should fulfill the properties of a notion of distance between polynomials, which in turn should reduce to the notion of distance between real numbers for 0-th order polynomials. The current abs method in TaylorSeries doesn't fulfill these properties; but norm(...,1), as is currently defined in this PR, does.

So, if on this branch we do:

using TaylorSeries

import Base: rtoldefault, isfinite, isapprox

# rtoldefault, isfinite and isapprox overloading for TaylorSeries
rtoldefault(::Type{T}) where T<:AbstractSeries = sqrt(eps())
isfinite(x::AbstractSeries) = !isinf(x)

function isapprox{T<:AbstractSeries}(x::T, y::T; rtol::Real=rtoldefault(x,y), atol::Real=0, nans::Bool=false)
    x == y || (isfinite(x) && isfinite(y) && norm(x-y,1) <= atol + rtol*max(norm(x,1), norm(y,1))) || (nans && isnan(x) && isnan(y))
end

t = Taylor1(25)
p = sin(t)
q = sin(t+eps())

Then p ≈ p, q ≈ q, p ≈ q returns (true, true, true).

The second option, re-defining abs in TaylorSeries, is what would be closest to @lbenet suggestion (getting isapprox "for free") and would be viable as well as far as this PR goes, but also would represent giving abs a whole different meaning in TaylorSeries. Still, if we pursue this idea, removing the current abs methods in TaylorSeries, and then doing

using TaylorSeries

import Base: rtoldefault, isfinite, abs

# rtoldefault, isfinite and abs overloading for TaylorSeries
rtoldefault(::Type{T}) where T<:AbstractSeries = sqrt(eps())
isfinite(x::AbstractSeries) = !isinf(x)
abs{T<:AbstractSeries}(x::T) = norm(x,1)

t = Taylor1(25)
p = sin(t)
q = sin(t+eps())

then, again, p ≈ p, q ≈ q, p ≈ q returns (true, true, true). I'd actually be more inclined towards this last option, but what do you guys think?

isapprox is not yet added @PerezHz, but it may be easily included if we all agree with the norm methods here.

I'm happy with the norm methods in this PR!

PerezHz reviewed

View reviewed changes

src/other_functions.jl Outdated

+              #norm
+              norm(x::AbstractSeries, p::Real=2) = norm( norm.(x.coeffs, p), p)
+              norm{T<:NumberNotSeries}(x::Taylor1{T}, p::Real=2) = norm(x.coeffs, p)

Contributor

PerezHz Sep 30, 2017

Cool!👌

Member

lbenet commented Sep 30, 2017

You are right @PerezHz, we do need specialized methods for isapprox.

I would then suggest to add somewhere in the documentation the distinction: abs returns a polynomial, while norm a non-negative number.

PerezHz added 9 commits

September 30, 2017 23:09


          Add isapprox and auxiliary methods

bf3cce6


          Add one variable tests for isapprox

f2fa051


          Fix isfinite

4580a26


          Add many variable tests

732177a


          Add NaN test for many variables isfinite

68904a3


          Add isapprox tests for mixtures; fix real method for types

344f6d7


          Small fix on tests

301a4f0


          Fix mixtures tests

6a64bcf


          Test isapprox for mixtures with BigFloats

28f137e

PerezHz mentioned this pull request

Addendum: isapprox blas-ko/TaylorSeries.jl#1

Merged

PerezHz and others added 3 commits

October 1, 2017 04:32


          Add docstrings

b0a3a7c


          Add docstrings and a couple tests with ≉

a7765e7


          Merge branch 'master' into norm

c2488ec

coveralls commented Oct 4, 2017

Coverage increased (+0.02%) to 95.954% when pulling 62e3c84 on blas-ko:norm into 3b68fb3 on JuliaDiff:master.

4 similar comments

coveralls commented Oct 4, 2017

Coverage increased (+0.02%) to 95.954% when pulling 62e3c84 on blas-ko:norm into 3b68fb3 on JuliaDiff:master.

coveralls commented Oct 4, 2017

Coverage increased (+0.02%) to 95.954% when pulling 62e3c84 on blas-ko:norm into 3b68fb3 on JuliaDiff:master.

coveralls commented Oct 4, 2017

Coverage increased (+0.02%) to 95.954% when pulling 62e3c84 on blas-ko:norm into 3b68fb3 on JuliaDiff:master.

coveralls commented Oct 4, 2017 •

edited

Loading

Coverage increased (+0.02%) to 95.954% when pulling 62e3c84 on blas-ko:norm into 3b68fb3 on JuliaDiff:master.

Contributor Author

blas-ko commented Oct 4, 2017

Travis failed again. Nevertheless, this time error log shows something different. There are tests that are not passing for Mac's julia:nightly as @PerezHz noticed 2 days ago. Error log shows

Tests for HomogeneousPolynomial and TaylorN: Error During Test
  Test threw an exception of type UnicodeError
  Expression: string(-xH) == " - 1 x₁"
  UnicodeError: invalid character index

and other 3 similar errors with UnicodeError: invalid character index.

Member

lbenet commented Oct 4, 2017

@dpsanders Do you know what is going on with travis for mac? This PR passes very nicely in linux, but not mac. Are strings treated differently in mac w.r.t. linux? My proposal is to merge this and address in another PR the compatibility issues related to mac.

Contributor Author

blas-ko commented Oct 4, 2017

My proposal is to merge this and address in another PR the compatibility issues related to mac.

I agree!

Contributor

dpsanders commented Oct 4, 2017

I don't know what's going on. I only saw that there have been certain issues with Mac on Travis, but haven't been following the discussion, sorry. You might want to write ask on discourse.julialang.org.

Contributor

PerezHz commented Oct 4, 2017

Just as a side note, since the last mac and linux builds were done with different julia nightlies, the difference may also be due to recent changes in julia 0.7.0-DEV:

travis linux julia version:

$ julia -e 'versioninfo()'
Julia Version 0.7.0-DEV.1775
Commit 55ec9fe (2017-09-13 21:22 UTC)

travis mac julia version:

$ julia -e 'versioninfo()'
Julia Version 0.7.0-DEV.2046
Commit 16139d054e (2017-10-04 06:00 UTC)

I agree to merge this and work out the nightlies failures for mac on a new PR!

Also, there's some discussion going on in JuliaLang/julia#23983 about the Base.Test->Test change; perhaps after that PR is merged and the travis julia versions are updated, in a couple of days (weeks?) it'll be possible to do using Base.Test as deprecated syntax.

Meanwhile, I'll try to work out an MWE regarding the UnicodeError with strings and post a question to Discourse...

Contributor

PerezHz commented Oct 5, 2017

Can confirm that, using julia version 0.7.0-DEV.1775, on my machine (OS X/macOS 10.10.5 Yosemite), TaylorSeries tests do pass; but using julia version 0.7.0-DEV.2046, they do not

lbenet reviewed

View reviewed changes

test/manyvariables.jl Outdated

+                  @test norm(a) == norm([3,4,6,8.0])
+                  @test norm(a,4) == sum([3,4,6,8.0].^4)^(1/4.)
+                  @test norm(a,Inf) == 8.
+                  @test norm((3.+4im)*x) == abs(3.+4im)

Member

lbenet Oct 5, 2017

I am checking what is going on with travis for julia 0.7-dev on mac, and on doing this, I just noticed that julia 0.7 complains about 3.+4im as ambiguous (due to parsing conflicts, essentially to know if you mean 3. + ... or 3 .+ ...). I suggest to write it as 3.0 + 4im. Can you change it? See JuliaLang/julia#19089.

Member

lbenet commented Oct 5, 2017

I've been checking the problem with the subscripts in julia 0.7 in a mac, and it seems the problem is with printing things like x₁, which is done programmatically. I sent a comment on something small related with this PR to @blas-ko ; once it is addressed (no need to correct this later), I'll merge this and see if I can solve the problem in 0.7.

Still, I don't understand why it works in linux but it doesn't in mac...

Contributor

PerezHz commented Oct 6, 2017

Still, I don't understand why it works in linux but it doesn't in mac...

I think the different behavior may be due to different julia versions in linux vs mac on travis

Member

lbenet commented Oct 6, 2017

I restarted the linux nighties job...

Contributor

PerezHz commented Oct 6, 2017

I think I was able to get a MWE of the UnicodeError problem and a solution, thanks to @Keno!

Using julia version 0.7.0-DEV.2046 I tried:

julia> str1 = "a "
"a "

julia> str2 = "a"
"a"

julia> str2 == str1[1:end-1] # works fine
true

julia> str1 = "₁ "
"₁ "

julia> str2 = "₁"
"₁"

julia> str2 == str1[1:end-1] # Unicode character; throws error
ERROR: UnicodeError: invalid character index
Stacktrace:
 [1] getindex(::String, ::UnitRange{Int64}) at ./strings/string.jl:292

So, as @dpsanders suggested, I asked on Discourse if this was a known issue; turns out there's a breaking change introduced by JuliaLang/julia#22572, which corresponds to julia version 0.7.0-DEV.1864. This change is actually documented in the Breaking changes section of NEWS.md. As was pointed out by @Keno on Discourse, this problem can be easily fixed by doing:

julia> str1 = "₁ "
"₁ "

julia> str2 = "₁"
"₁"

julia> str2 == str1[1:prevind(str1, end)]
true

So the issue should be solved if, on line 135 of src/printing.jl we did return strout[1:prevind(strout, end)] instead of return strout[1:end-1]... Just tried that change locally (macOS 10.10.5, julia 0.7.0-DEV.2046) and it works!

Member

lbenet commented Oct 6, 2017

Thanks a lot for the fix @PerezHz ! I also found a less elegant but equivalent solution; yours is simply better. Can you push the fix, and also address this comment ?


          Fix printing as suggested by @Keno; fix tests

1e26f3a

PerezHz mentioned this pull request

Two fixes blas-ko/TaylorSeries.jl#2

Merged

Contributor

PerezHz commented Oct 6, 2017

Sure! Just pushed the requested changes to @blas-ko's fork of TaylorSeries, where this PR is hosted

PerezHz and others added 3 commits

October 5, 2017 23:47


          Add one more fix

60bfc68


          trigger travis build

7ba8025


          Merge pull request #2 from PerezHz/norm

0d99c27

Two fixes

Contributor Author

blas-ko commented Oct 6, 2017

Just merged @PerezHz PR which addresses 3. -> 3.0 and string problem using [1:prevind(str1, end)]. Hope this finally solves everything.

Member

lbenet commented Oct 6, 2017

Let's see how travis reacts!

coveralls commented Oct 6, 2017 •

edited

Loading

Coverage increased (+0.02%) to 95.954% when pulling 0d99c27 on blas-ko:norm into 3b68fb3 on JuliaDiff:master.

Contributor

PerezHz commented Oct 6, 2017

Green lights!

lbenet merged commit 2111846 into JuliaDiff:master

Member

lbenet commented Oct 6, 2017

Merging! Thanks a lot to all for contributing to this!

blas-ko deleted the norm branch

October 6, 2017 15:24

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet