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

Fix inference regression on 1.7 #471

Merged
merged 2 commits into from
Dec 15, 2021
Merged

Fix inference regression on 1.7 #471

merged 2 commits into from
Dec 15, 2021

Conversation

N5N3
Copy link
Contributor

@N5N3 N5N3 commented Dec 14, 2021

Since AbstractInterpolation <: AbstractArray, there's no need to define ndims and eltype ourself.
gridtype is broken, (and unused), so I remove it.
itptype is modified to avoid the recursive call of supertype.

Update Interpolations.jl
@codecov
Copy link

codecov bot commented Dec 14, 2021

Codecov Report

Merging #471 (d517db8) into master (827895e) will increase coverage by 8.98%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #471      +/-   ##
==========================================
+ Coverage   76.42%   85.41%   +8.98%     
==========================================
  Files          26       26              
  Lines        1421     1741     +320     
==========================================
+ Hits         1086     1487     +401     
+ Misses        335      254      -81     
Impacted Files Coverage Δ
src/Interpolations.jl 79.36% <100.00%> (+11.82%) ⬆️
src/filter1d.jl 91.66% <0.00%> (-2.88%) ⬇️
src/io.jl 95.29% <0.00%> (-0.66%) ⬇️
src/deprecations.jl 2.56% <0.00%> (-0.38%) ⬇️
src/rewrite.jl 0.00% <0.00%> (ø)
src/lanczos/lanczos.jl 100.00% <0.00%> (ø)
src/chainrules/chainrules.jl 100.00% <0.00%> (ø)
src/monotonic/monotonic.jl 94.57% <0.00%> (+1.05%) ⬆️
src/gridded/indexing.jl 75.21% <0.00%> (+3.30%) ⬆️
src/iterate.jl 100.00% <0.00%> (+3.44%) ⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 827895e...d517db8. Read the comment docs.

@mkitti
Copy link
Collaborator

mkitti commented Dec 14, 2021

Also tracking JuliaLang/julia#43368
JuliaLang/julia#43296

@N5N3
Copy link
Contributor Author

N5N3 commented Dec 14, 2021

This PR should be enough as a temporary fix.
Do we need test for itptype? looks like it is used only in count_interp_dims.
Edit: Well, looks like the count_interp_dims api is also insufficient.

julia> itp = CubicSplineInterpolation((1:100,1:100),randn(100,100));

julia> Interpolations.count_interp_dims(typeof(itp))
ERROR: MethodError: no method matching count_interp_dims(::Type{BSpline{Cubic{Line{OnGrid}}}})
Closest candidates are:
  count_interp_dims(::Type{IT}, ::Any) where IT<:Interpolations.InterpolationType at C:\Users\MYJ\.julia\packages\Interpolations\3gTQB\src\Interpolations.jl:164  count_interp_dims(::Type{IT}, ::Any) where {N, IT<:Tuple{Vararg{Interpolations.InterpolationType, N}}} at C:\Users\MYJ\.julia\packages\Interpolations\3gTQB\src\Interpolations.jl:165
  count_interp_dims(::Type{BSI}, ::Any) where BSI<:Interpolations.BSplineInterpolation at C:\Users\MYJ\.julia\packages\Interpolations\3gTQB\src\b-splines\b-splines.jl:157
  ...
Stacktrace:
 [1] count_interp_dims(#unused#::Type{BSpline{Cubic{Line{OnGrid}}}}, n::Int64)
   @ Interpolations C:\Users\MYJ\.julia\packages\Interpolations\3gTQB\src\Interpolations.jl:164
 [2] count_interp_dims(#unused#::Type{Interpolations.Extrapolation{Float64, 2, ScaledInterpolation{Float64, 2, Interpolations.BSplineInterpolation{Float64, 2, OffsetArrays.OffsetMatrix{Float64, Matrix{Float64}}, BSpline{Cubic{Line{OnGrid}}}, Tuple{Base.OneTo{Int64}, Base.OneTo{Int64}}}, BSpline{Cubic{Line{OnGrid}}}, Tuple{UnitRange{Int64}, UnitRange{Int64}}}, BSpline{Cubic{Line{OnGrid}}}, Throw{Nothing}}})
   @ Interpolations C:\Users\MYJ\.julia\packages\Interpolations\3gTQB\src\Interpolations.jl:163
 [3] top-level scope
   @ REPL[21]:1

@N5N3 N5N3 mentioned this pull request Dec 14, 2021
@mkitti
Copy link
Collaborator

mkitti commented Dec 15, 2021

Can you write a test for the one line you added?

@N5N3
Copy link
Contributor Author

N5N3 commented Dec 15, 2021

test added

@mkitti
Copy link
Collaborator

mkitti commented Dec 15, 2021

Merging

@mkitti mkitti merged commit d4f602f into JuliaMath:master Dec 15, 2021
@N5N3 N5N3 deleted the 1.7reg branch December 15, 2021 15:12
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.

None yet

2 participants