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

Inference improvement. #472

Merged
merged 2 commits into from
Feb 21, 2022
Merged

Inference improvement. #472

merged 2 commits into from
Feb 21, 2022

Conversation

N5N3
Copy link
Contributor

@N5N3 N5N3 commented Dec 14, 2021

Fix #469. test added. (I only test it on master)

@codecov
Copy link

codecov bot commented Dec 14, 2021

Codecov Report

Merging #472 (f24a035) into master (d517db8) will decrease coverage by 9.03%.
The diff coverage is 33.33%.

❗ Current head f24a035 differs from pull request most recent head 2d069f8. Consider uploading reports for the commit 2d069f8 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #472      +/-   ##
==========================================
- Coverage   85.41%   76.37%   -9.04%     
==========================================
  Files          26       26              
  Lines        1741     1418     -323     
==========================================
- Hits         1487     1083     -404     
- Misses        254      335      +81     
Impacted Files Coverage Δ
src/b-splines/indexing.jl 49.50% <33.33%> (-16.67%) ⬇️
src/requires/unitful.jl 0.00% <0.00%> (-100.00%) ⬇️
src/nointerp/nointerp.jl 44.44% <0.00%> (-35.56%) ⬇️
src/utils.jl 41.66% <0.00%> (-33.02%) ⬇️
src/b-splines/linear.jl 73.33% <0.00%> (-26.67%) ⬇️
src/b-splines/constant.jl 76.00% <0.00%> (-24.00%) ⬇️
src/b-splines/b-splines.jl 68.13% <0.00%> (-21.87%) ⬇️
src/scaling/scaling.jl 78.37% <0.00%> (-12.21%) ⬇️
src/Interpolations.jl 67.54% <0.00%> (-11.83%) ⬇️
... 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 d4f602f...2d069f8. Read the comment docs.

@N5N3
Copy link
Contributor Author

N5N3 commented Dec 14, 2021

The inference problem of slot_substitute should be fixed for dim <= 7

using Interpolations
nx = 5
A = zeros(Float64, nx, nx, nx, nx, nx, nx, nx)
itp = interpolate(A, BSpline(Quadratic(Reflect(OnCell()))))
@code_warntype  Interpolations.hessian(itp, 1., 1., 1., 1., 1., 1., 1.)

shows

MethodInstance for Interpolations.hessian(::Interpolations.BSplineInterpolation{Float64, 7, OffsetArrays.OffsetArray{Float64, 7, Array{Float64, 7}}, BSpline{Quadratic{Reflect{OnCell}}}, NTuple{7, Base.OneTo{Int64}}}, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64)
  from hessian(itp::Interpolations.BSplineInterpolation{T, N, TCoefs, IT, Axs} where {TCoefs<:AbstractArray, IT<:Union{NoInterp, Tuple{Vararg{Union{NoInterp, BSpline}}}, BSpline}, Axs<:Tuple{Vararg{AbstractUnitRange, N}}}, x::Vararg{Number, N}) where {T, N} in Interpolations at c:\Users\MYJ\Documents\GitHub\Interpolations.jl\src\b-splines\indexing.jl:37
Static Parameters
  T = Float64
  N = 7      
Arguments
  #self#::Core.Const(Interpolations.hessian)
  itp::Interpolations.BSplineInterpolation{Float64, 7, OffsetArrays.OffsetArray{Float64, 7, Array{Float64, 7}}, BSpline{Quadratic{Reflect{OnCell}}}, NTuple{7, Base.OneTo{Int64}}}
  x::NTuple{7, Float64}
Locals
  #15::Interpolations.var"#15#16"{Interpolations.BSplineInterpolation{Float64, 7, OffsetArrays.OffsetArray{Float64, 7, Array{Float64, 7}}, BSpline{Quadratic{Reflect{OnCell}}}, NTuple{7, Base.OneTo{Int64}}}}
  wis::NTuple{28, NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}}
Body::StaticArrays.SMatrix{7, 7, Float64, 49}
1nothing
│         Core.NewvarNode(:(#15))
│         Core.NewvarNode(:(wis))
└──       goto #5 if not $(Expr(:boundscheck))
2%5  = Core.tuple(Interpolations.Bool, itp)::Core.PartialStruct(Tuple{DataType, Interpolations.BSplineInterpolation{Float64, 7, OffsetArrays.OffsetArray{Float64, 7, Array{Float64, 7}}, BSpline{Quadratic{Reflect{OnCell}}}, NTuple{7, Base.OneTo{Int64}}}}, Any[Type{Bool}, Interpolations.BSplineInterpolation{Float64, 7, OffsetArrays.OffsetArray{Float64, 7, Array{Float64, 7}}, BSpline{Quadratic{Reflect{OnCell}}}, NTuple{7, Base.OneTo{Int64}}}])
│   %6  = Core._apply_iterate(Base.iterate, Interpolations.checkbounds, %5, x)::Bool
└──       goto #4 if not %6
3 ─       goto #5
4%9  = Base.throw_boundserror::Core.Const(Base.throw_boundserror)
└──       (%9)(itp, x)
5%11 = Core.tuple(Interpolations.value_weights, Interpolations.gradient_weights, Interpolations.hessian_weights)::Core.Const((Interpolations.value_weights, Interpolations.gradient_weights, Interpolations.hessian_weights))
│   %12 = Core.tuple(%11)::Core.Const(((Interpolations.value_weights, Interpolations.gradient_weights, Interpolations.hessian_weights),))
│   %13 = Interpolations.itpinfo(itp)::Tuple{NTuple{7, BSpline{Quadratic{Reflect{OnCell}}}}, NTuple{7, Base.OneTo{Int64}}}
│   %14 = Core.tuple(x)::Tuple{NTuple{7, Float64}}
│         (wis = Core._apply_iterate(Base.iterate, Interpolations.weightedindexes, %12, %13, %14))
│   %16 = Interpolations.:(var"#15#16")::Core.Const(Interpolations.var"#15#16")
│   %17 = Core.typeof(itp)::Core.Const(Interpolations.BSplineInterpolation{Float64, 7, OffsetArrays.OffsetArray{Float64, 7, Array{Float64, 7}}, BSpline{Quadratic{Reflect{OnCell}}}, NTuple{7, Base.OneTo{Int64}}})
│   %18 = Core.apply_type(%16, %17)::Core.Const(Interpolations.var"#15#16"{Interpolations.BSplineInterpolation{Float64, 7, OffsetArrays.OffsetArray{Float64, 7, 
Array{Float64, 7}}, BSpline{Quadratic{Reflect{OnCell}}}, NTuple{7, Base.OneTo{Int64}}}})
│         (#15 = %new(%18, itp))%20 = #15::Interpolations.var"#15#16"{Interpolations.BSplineInterpolation{Float64, 7, OffsetArrays.OffsetArray{Float64, 7, Array{Float64, 7}}, BSpline{Quadratic{Reflect{OnCell}}}, NTuple{7, Base.OneTo{Int64}}}}
│   %21 = Interpolations.map(%20, wis::Core.PartialStruct(NTuple{28, NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}}, Any[Core.PartialStruct(NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}, Any[Core.PartialStruct(Interpolations.WeightedAdjIndex{3, Float64}, Any[Int64, Core.Const((1.0, -2.0, 1.0))]), Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, Float64}]), NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}, NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}, NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}, NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}, NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}, NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}, Core.PartialStruct(NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}, Any[Interpolations.WeightedAdjIndex{3, Float64}, Core.PartialStruct(Interpolations.WeightedAdjIndex{3, Float64}, Any[Int64, Core.Const((1.0, -2.0, 1.0))]), Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, 
Float64}, Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, Float64}]), NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}, NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}, NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}, NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}, NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}, Core.PartialStruct(NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}, Any[Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, Float64}, Core.PartialStruct(Interpolations.WeightedAdjIndex{3, Float64}, Any[Int64, Core.Const((1.0, -2.0, 1.0))]), Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, Float64}]), NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}, NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}, NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}, NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}, Core.PartialStruct(NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}, Any[Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, Float64}, Core.PartialStruct(Interpolations.WeightedAdjIndex{3, Float64}, Any[Int64, Core.Const((1.0, -2.0, 1.0))]), Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, Float64}]), NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}, NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}, NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}, Core.PartialStruct(NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}, Any[Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, Float64}, Core.PartialStruct(Interpolations.WeightedAdjIndex{3, Float64}, Any[Int64, Core.Const((1.0, -2.0, 1.0))]), Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, Float64}]), NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}, NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}, Core.PartialStruct(NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}, Any[Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, Float64}, Core.PartialStruct(Interpolations.WeightedAdjIndex{3, Float64}, Any[Int64, Core.Const((1.0, -2.0, 1.0))]), Interpolations.WeightedAdjIndex{3, Float64}]), NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}, Core.PartialStruct(NTuple{7, Interpolations.WeightedAdjIndex{3, Float64}}, Any[Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, Float64}, Interpolations.WeightedAdjIndex{3, Float64}, Core.PartialStruct(Interpolations.WeightedAdjIndex{3, Float64}, Any[Int64, Core.Const((1.0, -2.0, 1.0))])])]))::NTuple{28, Float64}
│   %22 = Interpolations.symmatrix(%21)::StaticArrays.SMatrix{7, 7, Float64, 49}
└──       return %22

I think it should be enough for most cases.

symmatrix was broken previously. fixed.

fix ambiguity

Update indexing.jl

add more type annotation
add test

only do extreme test when we have julia `power`

`^` was llvm based, thus `symmatrix` for 4x4, 5x5, etc is not stable until 1.7.

Update runtests.jl
@mkitti
Copy link
Collaborator

mkitti commented Dec 27, 2021

Is this now good to merge?

@N5N3
Copy link
Contributor Author

N5N3 commented Dec 27, 2021

I think it should be sufficient for most usage.
We can land it now and revisit it if someone want stability for higher dim.

(In fact the kernal for coefficient calulation for 7d case is really huge. (IIRC over 10000 line in code typed)
we’d better avoid over inline in such cases.)

@N5N3
Copy link
Contributor Author

N5N3 commented Jan 19, 2022

Bump?

@touste
Copy link

touste commented Feb 21, 2022

Hi, do you plan on merging this PR soon? I make heavy use of 3D hessians in my code and this fixes type stability.
Thanks!

@mkitti mkitti merged commit 8a89ddf into JuliaMath:master Feb 21, 2022
@mkitti
Copy link
Collaborator

mkitti commented Feb 21, 2022

Thanks for the reminder, @touste. Are you using this branch in production?

@touste
Copy link

touste commented Feb 22, 2022

Yes I was using this branch without any issue for a while, although I won't go so far as to say my code is "production" :-D

@mkitti
Copy link
Collaborator

mkitti commented Feb 22, 2022

The test seems to be failing as merged into master?

@N5N3
Copy link
Contributor Author

N5N3 commented Feb 23, 2022

Previous test was done on 1.7.0. I'll take a look today.

mkitti added a commit that referenced this pull request Feb 25, 2022
Fix inference on `symmatrix` (patch for #472)
@N5N3 N5N3 deleted the issue_469 branch February 25, 2022 01:41
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.

Type instability in hessian when interpolating a 3D field
3 participants