Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Solve invalidations #344

Merged
merged 6 commits into from
Dec 5, 2023
Merged

Solve invalidations #344

merged 6 commits into from
Dec 5, 2023

Conversation

lbenet
Copy link
Member

@lbenet lbenet commented Dec 3, 2023

Fixes #339

@coveralls
Copy link

coveralls commented Dec 3, 2023

Coverage Status

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

@lbenet
Copy link
Member Author

lbenet commented Dec 3, 2023

@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
Copy link
Member Author

lbenet commented Dec 3, 2023

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
Copy link
Member Author

lbenet commented Dec 4, 2023

@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
Copy link
Member Author

lbenet commented Dec 4, 2023

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
Copy link

lrnv commented Dec 4, 2023

@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
Copy link
Contributor

PerezHz commented Dec 4, 2023

@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 lbenet marked this pull request as ready for review December 4, 2023 19:10
@lbenet
Copy link
Member Author

lbenet commented Dec 4, 2023

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
Copy link
Member Author

lbenet commented Dec 4, 2023

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

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

@PerezHz
Copy link
Contributor

PerezHz commented Dec 4, 2023

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

Indeed!

@PerezHz
Copy link
Contributor

PerezHz commented Dec 4, 2023

@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
Copy link
Member Author

lbenet commented Dec 5, 2023

Merging!

@lbenet lbenet merged commit 9437c3f into master Dec 5, 2023
13 checks passed
@lbenet lbenet deleted the lb/iss339 branch February 4, 2024 03:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalidation
4 participants