Test with Aqua.jl #62
Conversation
|
This PR aims to check with Aqua.jl, specially for ambiguities, and issues with type piracy, which may affect performance |
|
Aqua reports some ambiguities, and possible test piracies (only in Julia v1.10). Note that I have commented several tests to see the output from Aqua (which strangely differs from what I get locally). |
|
Regarding the method ambiguities I think those come from Interpolations.jl, it's probably worth opening an issue there? |
some tests commented for insight
|
Rebased to current master... |
Remove Test compat entry
Remove Distributed compat entry
|
(I went ahead with a couple of commits to remove Test and Distributed compat entries) |
|
Also, just opened JuliaSpace/SatelliteToolboxTransformations.jl#6 which I hope can shed some light into how to avoid overloading |
This reverts commit 965e152.
|
Pushed a few commits which, together with JuliaDiff/TaylorSeries.jl#353, solve method ambiguities and a possible type piracy |
|
JuliaDiff/TaylorSeries.jl#354 solves another type piracy, but that's not strictly necessary as things are now, since piracy tests are marked as broken (we can either solve the piracies or leave those tests marked as broken for the time being). |
| (res::AbstractVector{OpticalResidual{T, TaylorN{T}}})(x::Vector{T}) where {T <: Real} = evaluate(res, x) | ||
| (res::Vector{OpticalResidual{T, TaylorN{T}}})(x::Vector{T}) where {T <: Real} = evaluate(res, x) |
There was a problem hiding this comment.
Naive question: what is the motivation for this change? (I think AbstractVector would allow applying this function to more types, e.g. SubArray{T,1} or SVector).
There was a problem hiding this comment.
This may perhaps not be relevant, but I thought it was easier to ask...
There was a problem hiding this comment.
This helps avoid an ambiguity with callable methods from Interpolations.jl which also dispatch on <:AbstractArray{T,N}; I agree it's perhaps a bit restrictive, and a more generic solution could be to define a OpticalResidualVector struct and define the callable methods over that struct. I would suggest to go with this approach for now, and revisit this if needed.
There was a problem hiding this comment.
Yet another possibility is to avoid using Vector{T} for the calling argument type (again, maybe wrapping it somehow in another struct)
|
Pushed a couple of commits reflecting the current state of things after merging of JuliaDiff/TaylorSeries.jl#354 |
|
There are still some type-piracies due to some TaylorInterpolant JLD2 serialization dispatches; since TaylorInterpolant lives in PlanetaryEphemeris that type-piracy can be solved by moving over those dispatches over to PlanetaryEphemeris (working on it) |
|
Solved the remaining type-piracies by moving over the corresponding methods to PlanetaryEphemeris. Are there any other thing you think we should add here @lbenet? |
|
Thanks a lot @PerezHz for solving all the remaining issues related to this PR. I am in favor of merging it as it is. |
No description provided.