JuliaDiff / TaylorSeries.jl

Taylor polynomial expansions in one and several independent variables.
Other
353 stars 53 forks source link

Solve invalidations #344

Closed lbenet closed 11 months ago

lbenet commented 11 months ago

Fixes #339

coveralls commented 11 months ago

Coverage Status

coverage: 97.198% (-0.009%) from 97.207% when pulling 4728ef616b66b5b85cfa3fe4db825dfeea673a57 on lb/iss339 into 9a16131fd1ba6a7616c99be084b5142b90b2ed77 on master.

lbenet commented 11 months ago

@lrnv Does this help to solve the invalidations? There are still some triggered when IntervalArithmetic is loaded; I'll try to solve them, but IntervalArithmetic is currently under deep revision...

lbenet commented 11 months ago

Following the code in this comment, using the code in this branch, I now get:

julia> show(trees[end])  # show the most invalidating method
inserting broadcasted(::Base.Broadcast.DefaultArrayStyle{N}, op, r::FillArrays.AbstractFill{T, N}) where {T, N} @ FillArrays ~/.julia/packages/FillArrays/2eiCe/src/fillbroadcast.jl:78 invalidated:
   backedges: 1: superseding broadcasted(::S, f, args...) where S<:BroadcastStyle @ Base.Broadcast broadcast.jl:1319 with MethodInstance for Base.Broadcast.broadcasted(::Base.Broadcast.BroadcastStyle, ::typeof(esc), ::Any) (21 children)
   1 mt_cache
false

So it seems it worked. I'll check the implications in TaylorIntegration before proceeding...

cc: @lrnv, @PerezHz

lbenet commented 11 months ago

@PerezHz, it seems that no problems occur in TaylorIntegration, as far as I checked the tests, but it would be nice if you can double check this. Can you do it?

lbenet commented 11 months ago

I checked the invalidations wrt IntervalArithmetic, and they are none due to TS. Yet, when loading IntervalArithmetic, we get some ambiguities that are detected by Aqua:

Aqua.detect_ambiguities(TaylorSeries; recursive = true)
2-element Vector{Tuple{Method, Method}}:
 (^(a::TaylorN{Interval{T}}, r::S) where {T<:Real, S<:Real} @ TaylorSeriesIAExt ~/.julia/dev/TaylorSeries/ext/TaylorSeriesIAExt.jl:46, ^(a::TaylorN, x::S) where S<:Rational @ TaylorSeries ~/.julia/dev/TaylorSeries/src/power.jl:50)
 (^(a::Taylor1{Interval{T}}, r::S) where {T<:Real, S<:Real} @ TaylorSeriesIAExt ~/.julia/dev/TaylorSeries/ext/TaylorSeriesIAExt.jl:22, ^(a::Taylor1, x::S) where S<:Rational @ TaylorSeries ~/.julia/dev/TaylorSeries/src/power.jl:50)
lrnv commented 11 months ago

@lbenet Looking at your code I do not understand why, but indeed it solved the invalidations I was having. GG !

Number of invals reduced from 502 to 192, which is great. The remaining ones are NOT from TaylorSeries.jl, so you are out of the woods: I'll wait for this PR to get merged and versionned and then I'll take these invalidations somewhere else :)

If you need something else from me do not hesitate to ping me again. Thanks a lot !

PerezHz commented 11 months ago

@PerezHz, it seems that no problems occur in TaylorIntegration, as far as I checked the tests, but it would be nice if you can double check this. Can you do it?

Just double-checked and everything seems to be fine! 👍

lbenet commented 11 months ago

Looking at your code I do not understand why, but indeed it solved the invalidations I was having. GG !

@lrnv I only commented out all the lines that were pointed out by SnoopCompile; I do not understand the invalidations, because the methods seemed quite concrete to me... Anyway, I also noticed that there were some issues related to _InternalMutFuncs which were easy to solve.

There are still some ambiguities which appear when IntervalArithmetic is loaded; I'll try to solve them in the next few days, but I do not consider them crutial because IntervalArithmetic is being rewritten; if I don't solve them, they are left for some near future.

lbenet commented 11 months ago

Just double-checked and everything seems to be fine! 👍

Excellent! I guess this holds for PlanetaryEphemeris and NEOs...

PerezHz commented 11 months ago

Excellent! I guess this holds for PlanetaryEphemeris and NEOs...

Indeed!

PerezHz commented 11 months ago

@lbenet: just to be clear on my previous comment, PlanetaryEphemeris and NEOs tests pass using this branch of TaylorSeries (a very small issue appeared on the latter, but that's completely unrelated to this PR).

lbenet commented 11 months ago

Merging!