Improve `Taylor1{TaylorN{T}}` mixtures #333

lbenet · 2023-08-11T18:42:06Z

This PR defines some specialized methods on Taylor1{TaylorN{T}}. This specialization is aimed to improve jet transport Taylor integrations.

coveralls · 2023-08-11T18:45:45Z

coverage: 98.097% (+0.9%) from 97.198%
when pulling 088c1c3 on lb/improve_mixtures2
into 36586d7 on master.

PerezHz

Many thanks @lbenet for adding these improvements to the handling of mixtures! Looks like this will really help TaylorIntegration.jl and NEOs.jl. I left a couple of comments about some minor stuff, but otherwise LGTM!

src/arithmetic.jl

PerezHz · 2023-09-03T18:44:52Z

src/arithmetic.jl

+        # function ($f)(a::Taylor1{TaylorN{T}}, b::Taylor1{TaylorN{S}}) where
+        #         {T<:NumberNotSeries, S<:NumberNotSeries}
+        #     R = promote_type(T,S)
+        #     return ($f)(convert(Taylor1{TaylorN{R}}, a), convert(Taylor1{TaylorN{R}}, b))
+        # end
+
+        # function ($f)(a::Taylor1{TaylorN{T}}, b::Taylor1{TaylorN{T}}) where
+        #         {T<:NumberNotSeries}
+        #     if (a.order != b.order) || any(get_order.(a[:]) .!= get_order.(b[:]))
+        #         a, b = fixorder(a, b)
+        #     end
+        #     res = Taylor1(zero( constant_term(a)), a.order )
+        #     for ordT in eachindex(a)
+        #         ($fc)(res, a, b, ordT)
+        #     end
+        #     return res
+        # end
+
+        # function ($fc)(res::Taylor1{TaylorN{T}}, a::Taylor1{TaylorN{T}},
+        #         b::Taylor1{TaylorN{T}}, ordT::Int) where {T<:NumberNotSeries}
+        #     # Sanity
+        #     zero!(res, a, ordT)
+        #     @inbounds for ordQ in eachindex(a[ordT])
+        #         $(fc)(res[ordT], a[ordT], b[ordT], ordQ)
+        #     end
+        #     return nothing
+        # end


Are these lines intended to stay commented as they are, or does it make sense for them to be deleted?

I think I wanted to have more specific methods, but something didn't work (ambiguities?). For the time being I'll leave them there, but if they are not needed, I'll remove them.

src/arithmetic.jl

PerezHz · 2023-09-03T18:47:06Z

src/arithmetic.jl

+        # v = Array{typeof(aux)}(undef, length(a.coeffs))
+        # @__dot__ v = a.coeffs / b
+        # return $T(v, a.order)


Looks like these commented lines are meant to be removed?

Yes, they should be eventually removed. I think I kept them to check performance. I shall keep this conversation open, to remember eventually remove them if not needed.

PerezHz · 2023-09-03T18:47:29Z

src/arithmetic.jl

+        # R = typeof(aux)
+        # coeffs = Array{R}(undef, length(b.coeffs))
+        # @__dot__ coeffs = b.coeffs / a
+        # return Taylor1(coeffs, b.order)


Same comment as above

src/arithmetic.jl

src/auxiliary.jl

PerezHz · 2023-09-04T12:34:38Z

If I remember correctly, I think it was suggested in an offline discussion that it probably makes sense to add specialized evaluate methods for mixtures, is that still the case?

lbenet · 2023-12-06T03:28:49Z

If I remember correctly, I think it was suggested in an offline discussion that it probably makes sense to add specialized evaluate methods for mixtures, is that still the case?

Sorry for the long delay to get back to your comments... I have implemented most of your suggestions. About the comments, I think I left them there because I was testing improving performance, but either didn't get better results, or got some sort of ambiguities/errors. I left those conversations unresolved, to come back later to them.

PerezHz · 2023-12-06T22:39:15Z

Many thanks for addressing the comments! I agree that probably there's room left to improve performance, probably we can merge this as a first step and then try to get further performance gains in future PRs. Thus, as it stands now, it's probably worth bumping at least the patch version?

lbenet · 2024-01-28T19:13:11Z

I've removed some methods of evaluate, restricting their application, that I think had some impact in performance, but that is difficult to judge that. I think I still need to add some tests, and then it should be ready.

In pass, I also checked if there were invalidations triggered by using TaylorSeries; there are none. Once this is merged, I'll check the impact in Taylorintegration.jl

lbenet · 2024-01-29T22:32:34Z

This follows the discussion from PerezHz/PlanetaryEphemeris.jl#30 (comment)

The following is the example used, using this branch

julia> using TaylorSeries

julia> x = Taylor1.([rand(3) for _ in 1:5, __ in 1:2])
5×2 Matrix{Taylor1{Float64}}:
    0.0991832863555554 + 0.6683889089955399 t + 0.7967925292561205 t² + 𝒪(t³)  …      0.489775369583631 + 0.1409952219760241 t + 0.7373398449922951 t² + 𝒪(t³)
...

julia> dq = TaylorSeries.set_variables("dq", order=2, numvars=2);

julia> x(3.04 + dq[1])
5×2 Matrix{TaylorN{Float64}}:
      9.494723408075362 + 5.512887486872753 dq₁ + 0.7967925292561205 dq₁² + 𝒪(‖x‖³)  …    7.732600755871539 + 4.624021479529178 dq₁ + 0.7373398449922951 dq₁² + 𝒪(‖x‖³)
...

julia> @code_warntype x(3.04 + dq[1])
MethodInstance for (::Matrix{Taylor1{Float64}})(::TaylorN{Float64})
  from (p::Array{Taylor1{T}})(x) where T<:Number @ TaylorSeries ~/.julia/dev/TaylorSeries/src/evaluate.jl:102
Static Parameters
  T = Float64
Arguments
  p::Matrix{Taylor1{Float64}}
  x::TaylorN{Float64}
Body::Any
1 ─ %1 = Base.broadcasted(TaylorSeries.evaluate, p, x)::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{2}, Nothing, typeof(evaluate), Tuple{Matrix{Taylor1{Float64}}, TaylorN{Float64}}}
│   %2 = Base.materialize(%1)::Any
└──      return %2

julia> @which evaluate(x, 3.04 + dq[1])
evaluate(x::AbstractArray{Taylor1{T}}, δt::S) where {T<:Number, S<:Number}
     @ TaylorSeries ~/.julia/dev/TaylorSeries/src/evaluate.jl:42

The method quoted uses broadcasting, so I guess that is the problem.

lbenet · 2024-01-31T14:29:18Z

@PerezHz I think I have solved the issue you reported. I have checked that all the changes do not induce invalidations, and that tests pass in TaylorIntegration. In my opinion this is ready to be merged, except for tagging the new version; I am in favor of bumping a new minor version. Do you see anything that still has to be addressed?

PerezHz · 2024-01-31T18:01:30Z

@lbenet LGTM! I think there's further improvements to be made in the in-place methods like mul! for mixtures, but we can address this in future PRs (I have an idea regarding this in the works, will try to upload a very WIP proof-of-concept PR today)

lbenet · 2024-01-31T18:24:02Z

Great! So I'll bump the patch version, merge, and then release the new version!

for Taylor1{TaylorN{T}}

and fix tests

(fixing certain type instability)

PerezHz mentioned this pull request Aug 13, 2023

Minor scattered updates PerezHz/NEOs.jl#33

Merged

PerezHz reviewed Sep 3, 2023

View reviewed changes

lbenet force-pushed the lb/improve_mixtures2 branch from be646bb to 0c5c8c0 Compare December 5, 2023 15:19

lbenet force-pushed the lb/improve_mixtures2 branch 2 times, most recently from 9e17ecc to 09e773e Compare December 14, 2023 16:36

lbenet mentioned this pull request Jan 29, 2024

Fix interpolation PerezHz/PlanetaryEphemeris.jl#30

Merged

lbenet added 15 commits January 31, 2024 12:51

Add tests for Taylor1{TaylorN{T}}

b7650a9

Add fixorder to Taylor1{TaylorN{T}}

83d0771

Add methods for arithm opers for Taylor1{TaylorN{T}}

bf87a3f

Add methods of ^, sqrt, pow!, sqr!, sqrt! for Taylor1{TaylorN{T}

bed81a1

Add specialized methods for most mutating functions

ebf3584

for Taylor1{TaylorN{T}}

Implement suggestions by @PerezHz

e9dd41e

Fix error in docs

929ba94

Use TS

1088dc0

Improvements for evaluate

9ac87e9

Clean up commented (old) code

7bbe81a

Remove some superfluous methods in evaluate

75f1be3

More fixes

2c68db8

Fixes and clean-up of evaluate methods

39cdf07

Returning a zero Homogeneouspolynomial is of order 0

d710792

Scattered fixes and clean up

54a6523

lbenet added 6 commits January 31, 2024 12:51

Fix documentation

9b4206a

Remove evaluate methods for TaylorN with Varargs,

c1f8b9e

and fix tests

Fix _evaluate for mixtures, and new tests

66935ac

Methods of evaluate for Taylor1 involving TaylorN

77846cf

(fixing certain type instability)

Add tests for new evaluate methods

8d2876e

Bump minor version

a4a2e4c

lbenet force-pushed the lb/improve_mixtures2 branch from 94a41e4 to a4a2e4c Compare January 31, 2024 18:51

Remove a @show left there

088c1c3

lbenet merged commit f5587a7 into master Jan 31, 2024
13 checks passed

lbenet deleted the lb/improve_mixtures2 branch February 4, 2024 03:25

lbenet mentioned this pull request Feb 25, 2024

What was breaking in 0.16 and 0.17 ? #348

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `Taylor1{TaylorN{T}}` mixtures #333

Improve `Taylor1{TaylorN{T}}` mixtures #333

lbenet commented Aug 11, 2023

coveralls commented Aug 11, 2023 •

edited

Loading

PerezHz left a comment

PerezHz Sep 3, 2023

lbenet Dec 6, 2023

PerezHz Sep 3, 2023

lbenet Dec 6, 2023

PerezHz Sep 3, 2023

PerezHz commented Sep 4, 2023

lbenet commented Dec 6, 2023

PerezHz commented Dec 6, 2023

lbenet commented Jan 28, 2024

lbenet commented Jan 29, 2024

lbenet commented Jan 31, 2024

PerezHz commented Jan 31, 2024

lbenet commented Jan 31, 2024

Improve Taylor1{TaylorN{T}} mixtures #333

Improve Taylor1{TaylorN{T}} mixtures #333

Conversation

lbenet commented Aug 11, 2023

coveralls commented Aug 11, 2023 • edited Loading

PerezHz left a comment

Choose a reason for hiding this comment

PerezHz Sep 3, 2023

Choose a reason for hiding this comment

lbenet Dec 6, 2023

Choose a reason for hiding this comment

PerezHz Sep 3, 2023

Choose a reason for hiding this comment

lbenet Dec 6, 2023

Choose a reason for hiding this comment

PerezHz Sep 3, 2023

Choose a reason for hiding this comment

PerezHz commented Sep 4, 2023

lbenet commented Dec 6, 2023

PerezHz commented Dec 6, 2023

lbenet commented Jan 28, 2024

lbenet commented Jan 29, 2024

lbenet commented Jan 31, 2024

PerezHz commented Jan 31, 2024

lbenet commented Jan 31, 2024

Improve `Taylor1{TaylorN{T}}` mixtures #333

Improve `Taylor1{TaylorN{T}}` mixtures #333

coveralls commented Aug 11, 2023 •

edited

Loading