norm methods for AbstractSeries

[blas-ko]

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

[lbenet]

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?

[lbenet]

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

[blas-ko]

Great, I'll do this in a moment!

[lbenet]

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.

[blas-ko]

I'll change this too!

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[lbenet]

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

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

[lbenet]

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

[blas-ko]

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!

[lbenet]

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.

[lbenet]

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

[PerezHz]

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:

1. overloading isapprox for TaylorSeries polynomials
2. 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]

Cool!👌

[lbenet]

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.

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[blas-ko]

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.

[lbenet]

@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.

[blas-ko]

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

I agree!

[dpsanders]

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.

[PerezHz]

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...

[PerezHz]

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]

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.

[lbenet]

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...

[PerezHz]

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

[lbenet]

I restarted the linux nighties job...

[PerezHz]

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!

[lbenet]

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 ?

[PerezHz]

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

[blas-ko]

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

[lbenet]

Let's see how travis reacts!

[coveralls]

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

[PerezHz]

Green lights!

[lbenet]

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