Small fix in Taylor1 constructor?

[PerezHz]

This PR is a bit related to #342 and #343 and represents a solution to the following (unexpected?) behavior: if we have a Taylor1{TaylorN{Float64}} constructed from another variable of the same type, modifying in-place the former modifies also the latter:

julia> using TaylorSeries

julia> set_variables("x", numvars=2, order=2)
2-element Vector{TaylorN{Float64}}:
  1.0 x₁ + 𝒪(‖x‖³)
  1.0 x₂ + 𝒪(‖x‖³)

julia> a = sum(exp.(get_variables()).^2)
 2.0 + 2.0 x₁ + 2.0 x₂ + 2.0 x₁² + 2.0 x₂² + 𝒪(‖x‖³)

julia> b = Taylor1([a])
  2.0 + 2.0 x₁ + 2.0 x₂ + 2.0 x₁² + 2.0 x₂² + 𝒪(‖x‖³) + 𝒪(t¹)

julia> bcopy = deepcopy(b)
  2.0 + 2.0 x₁ + 2.0 x₂ + 2.0 x₁² + 2.0 x₂² + 𝒪(‖x‖³) + 𝒪(t¹)

julia> c = Taylor1(constant_term(b),0)
  2.0 + 2.0 x₁ + 2.0 x₂ + 2.0 x₁² + 2.0 x₂² + 𝒪(‖x‖³) + 𝒪(t¹)

julia> c[0][0][1] = 0.0
0.0

julia> b == bcopy # I would expect this to be true, since only c was modified
false

What are your thoughts over this @lbenet?

[coveralls]

coverage: 97.198%. remained the same
when pulling 3793597 on PerezHz:jp/constructor-fix
into 9437c3f on JuliaDiff:master.

[lbenet]

Good catch @PerezHz, and sorry for the delay to get into it...

In general, I like the simplicity of the proposed solution, and agree to merge this. Before that, let me ask some questions: Do you know if there is a similar example/problem for Taylor1{T} for T<:NumberNotSeries, and what is the impact that deepcopy has in such cases? In other words, can we avoid the deepcopy (in some cases, if it impacts performace/allocations), and only use it for the mixed structs? Do we also need something similar for TaylorN{Taylor1{T}}?

[lbenet]

Just to have it written somewhere, I think this issue is due to the views: induced e.g. by these lines.

[PerezHz]

Indeed, on current main, there's similar behavior for Taylor1{T} where T<:NumberNotSeries and TaylorN{Taylor1{T}}:

julia> using TaylorSeries

julia> set_variables("x", numvars=2, order=2)
2-element Vector{TaylorN{Float64}}:
  1.0 x₁ + 𝒪(‖x‖³)
  1.0 x₂ + 𝒪(‖x‖³)

julia> x = TaylorN(Taylor1(5),0)
 ( 1.0 t + 𝒪(t⁶)) + 𝒪(‖x‖¹)

julia> y = x
 ( 1.0 t + 𝒪(t⁶)) + 𝒪(‖x‖¹)

julia> y[0][1][0] = 7.1
7.1

julia> x
 ( 7.1 + 1.0 t + 𝒪(t⁶)) + 𝒪(‖x‖¹)

julia> x = Taylor1(rand(5))
 0.6049707581312165 + 0.175608853487092 t + 0.16818194777538198 t² + 0.6251617825278187 t³ + 0.8179378046391547 t⁴ + 𝒪(t⁵)

julia> y = x
 0.6049707581312165 + 0.175608853487092 t + 0.16818194777538198 t² + 0.6251617825278187 t³ + 0.8179378046391547 t⁴ + 𝒪(t⁵)

julia> y[0] = 1.0
1.0

julia> x
 1.0 + 0.175608853487092 t + 0.16818194777538198 t² + 0.6251617825278187 t³ + 0.8179378046391547 t⁴ + 𝒪(t⁵)

And if I remove the views from the getindex methods, then the problem seems to persist...

[PerezHz]

and what is the impact that deepcopy has in such cases? In other words, can we avoid the deepcopy (in some cases, if it impacts performace/allocations), and only use it for the mixed structs?

I agree that in general it's best to avoid allocations as much as possible, but If I think about how we use Taylor1 in TaylorIntegration, there we first allocate all the variables we need, and then start modifying their coefficients in-place throughout an integration. In that case, I think it actually makes sense to have the deepcopy at construction, since we are constructing a bunch of Taylor1s at the allocation stage which are in principle fully independent from each other (memory-wise); otherwise, we might end up modifying coefficients that were not meant to be updated. That said, if deemed a better solution, I can add a specialized constructor for mixed structs here which uses deepcopy, while leaving the constructor for Taylor1{T} where T<:NumberNotSeries as it is now (i.e., without deepcopy).

[lbenet]

I think it actually makes sense to have the deepcopy at construction, since we are constructing a bunch of Taylor1s at the allocation stage which are in principle fully independent from each other (memory-wise).

I fully agree with you. Let us go then that way. Can you also fix the TaylorN{Taylor1{T}} case, and bump a patch version?

[lbenet]

Merging this; the patch will come eventually...

Thanks a lot @PerezHz

[PerezHz]

@lbenet thanks for merging and sorry for delay with the patch, I'll try to come back around this in the next few days!

[lbenet]

Nevermind... we can always bump the version later!