Fuse operations and methods for mixed types #95

lbenet · 2017-03-10T14:55:52Z

This PR began as an attempt to get rid of the use fixorder, which creates copies of Taylor1 objects. This has been accomplished to a certain extent using dot-operatotions and dot-functions. But I couldn't get rid of some cases.

While working on this, #92 appeared followed by #94. I think I have solved this now by introducing new methods, and restricting more carefully some methods according to their methods.

In == we avoid using fixorder...

…onstructors Also, renamed check_taylor1_order! -> resize_coeffs1! and check_hpoly_order! -> resize_coeffsHP!

coveralls · 2017-03-10T15:34:27Z

Coverage decreased (-2.07%) to 96.204% when pulling fff6d00 on fuse_operations into b7eb18f on master.

lbenet · 2017-03-10T16:45:31Z

Pushed another commit to increase coverage

lbenet · 2017-03-10T17:27:29Z

To my surprise (at least in Julia 0.5), this PR seems to solve the cases described in #62 and #78 related to the order of loading TaylorSeries and another package.

Here is an example:

julia> using TaylorSeries, ArbFloats

julia> t1=Taylor1([ArbFloat(2), ArbFloat(3)])
 2 + 3 t + 𝒪(t²)

julia> exp(t1)
 7.389056098930650227230427 + 22.16716829679195068169128 t + 𝒪(t²)

and the other way around

julia> using ArbFloats, TaylorSeries

julia> t1=Taylor1([ArbFloat(2), ArbFloat(3)])
 2 + 3 t + 𝒪(t²)

julia> exp(t1)
 7.389056098930650227230427 + 22.16716829679195068169128 t + 𝒪(t²)

@PerezHz @JeffreySarnoff @fhenneke Can you confirm this is indeed the case?

coveralls · 2017-03-10T17:34:01Z

Coverage decreased (-1.4%) to 96.847% when pulling 61af53a on fuse_operations into b7eb18f on master.

PerezHz · 2017-03-10T17:58:17Z

Wow, quite unexpected! On my machine, the order of loading seems to be fixed for ArbFloats, ForwardDiff, and ValidatedNumerics!

coveralls · 2017-03-10T19:48:06Z

Coverage decreased (-0.05%) to 98.222% when pulling 025cba8 on fuse_operations into b7eb18f on master.

coveralls · 2017-03-10T20:45:48Z

Coverage increased (+0.4%) to 98.626% when pulling e23dec7 on fuse_operations into b7eb18f on master.

lbenet · 2017-03-10T21:02:00Z

@dpsanders Do you want to take a look on this PR? (Sorry for making it so complicate.) Tests pass and have been enlarged to cover other situations (#94), including unexpected ones (#62 and #78).

dpsanders · 2017-03-11T04:10:42Z

Yes, but I won't be able to take a proper look at it until Sunday.
If you prefer to merge now then go ahead.

lbenet · 2017-03-11T05:23:17Z

Take your time. You have always good comments and insight! Thanks!

dpsanders · 2017-03-12T19:43:04Z

Hmm, I see that not using fixorder does complicate the code quite a lot.

It's also a bit unfortunate that what was now relatively simple code for Taylor1 is mixed up with more complicated code for TaylorN?

lbenet · 2017-03-12T19:45:23Z

I agree, though your idea in #96 may help a little bit...

dpsanders

In general looks very good.
There are a few suggestions to simplify syntax, and a few places where it feels like some refactoring could be useful, which I have pointed out.

But these can also be postponed for another PR.

dpsanders · 2017-03-12T19:04:37Z

src/auxiliary.jl

@@ -8,27 +8,35 @@

 ## Auxiliary function ##

-function check_taylor1_order!{T<:Number}(coeffs::Array{T,1}, order::Int)
+"""
+    resize_coeffs1!{T<Number}(coeffs::Array{T,1}, order::Int)


With the changes to be able to handle Taylors of different orders, is this function necessary?

I moved part of the constructor of Taylor1 to this function to simplify the code. I'm not sure how to answer the question, since this function resizes the coefficients if you ask for a larger order. For instannce, you may call Taylor1([0,1],10) and at some point this function is called to resize the coefficients to size 11.

My point is that there will no longer be any need to resize the coefficients, ever!

I truly hope so!

dpsanders · 2017-03-12T19:05:02Z

src/auxiliary.jl

-        @inbounds coeffs[i] = zero(coeffs[1])
-    end
-    nothing
+    coeffs[lencoef+1:order+1] .= zero(coeffs[1])


`coeffs[lencoef+1:end]

I guess it should work... I'll push another commit with this change.

dpsanders · 2017-03-12T19:07:03Z

src/constructors.jl

+
+
+# A `Number` which is not an `AbstractSeries`
+const NumberNotSeries = Union{setdiff(subtypes(Number), [AbstractSeries])...}


This is clever!

Somehow beyond my understanding, I think this is at the core why problems reported in #62 and #94 are solved.... or Julia-magic.

dpsanders · 2017-03-12T19:30:47Z

src/evaluate.jl

    R = promote_type(T,S)
+    numVars = get_numvars()
    suma = convert(TaylorN{R}, a)


I don't understand why there is this convert here.

Type stability; if you simply use assignment (suma = a) it may change type inside the loop.

dpsanders · 2017-03-12T19:31:09Z

src/evaluate.jl

    suma = convert(TaylorN{R}, a)

-    for nv = 1:numVars
+    @inbounds for nv = 1:numVars
        suma = horner(suma, (nv, vals[nv]))
    end

    return suma.coeffs[1].coeffs[1]


It feels like this is unnecessary and suma should just be of the final type.

Maybe. The way it is done here is that a TaylorN polynomial is evaluated substituting each variable, one by one; maybe it's not the best way of doing it...

dpsanders · 2017-03-12T20:21:58Z

src/arithmetic.jl

+        end
+        function $f(a::HomogeneousPolynomial)
+            v = similar(a.coeffs)
+            v .= $f(a.coeffs)


These two lines can be reduced to v = $f(a.coeffs).

I guess so... Thanks for the catch!

dpsanders · 2017-03-12T20:22:40Z

src/arithmetic.jl

-            end
+            R = eltype(aux)
+            coeffs = Array{HomogeneousPolynomial{Taylor1{R}}}(a.order+1)
+            coeffs .= a.coeffs


These two lines should be equivalent to a convert?

dpsanders · 2017-03-12T20:24:29Z

src/arithmetic.jl

+            @inbounds aux = a * b.coeffs[1]
+            R = typeof(aux)
+            coeffs = Array{R}(length(b.coeffs))
+            coeffs .= a .* b.coeffs


It feels like these three lines form a recurring pattern that should be factored out into a function somehow.

I guess they appear like that to assure type stability.

dpsanders · 2017-03-12T20:25:02Z

src/arithmetic.jl

+            @inbounds aux = a * b.coeffs[1]
+            R = typeof(aux)
+            coeffs = Array{R}(length(b.coeffs))
+            coeffs .= a .* b.coeffs


There is a lot of repetition here with the previous definitions.

dpsanders · 2017-03-12T20:26:00Z

src/arithmetic.jl

@@ -262,7 +292,7 @@ Inputs are the `kcoef`-th coefficient, and the vectors of the expansion coeffici
 """
 function mulHomogCoef{T<:Number}(kcoef::Int, ac::Array{T,1}, bc::Array{T,1})
    kcoef == 0 && return ac[1] * bc[1]
-    coefhomog = zero(T)
+    coefhomog = zero(ac[1])


Same as some comment above; the difference appears for Taylor1{TaylorN{T}}...

lbenet · 2017-03-13T03:57:19Z

I'll revise the code and push another commit.

lbenet · 2017-03-13T14:31:45Z

@dpsanders Thanks for your review! And, again, I'm sorry that this PR became so extended and messy.

I just pushed another commit which address many of your observations, but not all. As you suggest, this can be part of another commit. Once we get green lights, I think this should be merged.

Somewhere, which i can't find now, you also suggested that the tests should reflect more the file that they are actually testing. I will reorganize them as well, but in another PR.

coveralls · 2017-03-13T14:48:18Z

Coverage increased (+0.4%) to 98.625% when pulling 60c9421 on fuse_operations into b7eb18f on master.

lbenet · 2017-03-13T17:36:17Z

Got green lights. Merging.

dpsanders · 2017-03-13T18:14:35Z

Nice!

Luis Benet added 8 commits March 9, 2017 17:16

Add Compat, import iszero (version dependent) and rewrite ==

cbad68d

In == we avoid using fixorder...

Fix arithmetic operations to get rid of fixorder

8ce5e7e

Revert some changes in evaluate and change slightly fixorder

e0adc57

Improve zero, one and /

9578099

Add AbstractSeries as an abstract container and improvements in the c…

5b4f0f9

…onstructors Also, renamed check_taylor1_order! -> resize_coeffs1! and check_hpoly_order! -> resize_coeffsHP!

Methods to handle the arithmetics of Taylor1{TaylorN{T}}

e509685

Adapt AbstractSeries to Julia 0.6 syntax and fix evaluate

53a5699

Fix warnings in Julia 0.6 related to isnan. and isinf. and add tests

fff6d00

Add tests and correct evaluate(::HomogeneousPolynomial)

61af53a

Another couple of tests

025cba8

Solve another tiny issue, and add tests

e23dec7

lbenet mentioned this pull request Mar 12, 2017

WIP: Improve allocation PerezHz/TaylorIntegration.jl#18

Merged

dpsanders mentioned this pull request Mar 12, 2017

Define getindex for Taylor #96

Closed

dpsanders approved these changes Mar 12, 2017

View reviewed changes

Address most suggestions of the code review

60c9421

lbenet merged commit a7fd782 into master Mar 13, 2017

This was referenced Mar 13, 2017

code should not require three argument multiply #62

Closed

the order of importing TalyorSeries should not matter #78

Closed

Problems with promotion of mixed-types #94

Closed

Error while loading MacroTools.jl and TaylorSeries.jl FluxML/MacroTools.jl#22

Closed

lbenet deleted the fuse_operations branch March 29, 2017 18:41

This was referenced Oct 19, 2017

Importing order for TaylorSeries and ArbFloats doesn't matter anymore JuliaArbTypes/ArbFloats.jl#21

Merged

Importing order for TaylorSeries and ArbFloats doesn't matter anymore JuliaArbTypes/ArbFloats.jl#22

Merged



		# A `Number` which is not an `AbstractSeries`
		const NumberNotSeries = Union{setdiff(subtypes(Number), [AbstractSeries])...}

Fuse operations and methods for mixed types #95

Fuse operations and methods for mixed types #95

Conversation

lbenet commented Mar 10, 2017

coveralls commented Mar 10, 2017 • edited Loading

lbenet commented Mar 10, 2017

lbenet commented Mar 10, 2017 • edited Loading

coveralls commented Mar 10, 2017 • edited Loading

PerezHz commented Mar 10, 2017

coveralls commented Mar 10, 2017 • edited Loading

coveralls commented Mar 10, 2017 • edited Loading

lbenet commented Mar 10, 2017

dpsanders commented Mar 11, 2017

lbenet commented Mar 11, 2017

dpsanders commented Mar 12, 2017

lbenet commented Mar 12, 2017

dpsanders left a 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

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

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

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

lbenet commented Mar 13, 2017

lbenet commented Mar 13, 2017

coveralls commented Mar 13, 2017 • edited Loading

lbenet commented Mar 13, 2017

dpsanders commented Mar 13, 2017

coveralls commented Mar 10, 2017 •

edited

Loading

lbenet commented Mar 10, 2017 •

edited

Loading

coveralls commented Mar 10, 2017 •

edited

Loading

coveralls commented Mar 10, 2017 •

edited

Loading

coveralls commented Mar 10, 2017 •

edited

Loading

coveralls commented Mar 13, 2017 •

edited

Loading