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

Generalize storage of factorizations. #42594

Merged
merged 1 commit into from
Feb 12, 2022
Merged

Generalize storage of factorizations. #42594

merged 1 commit into from
Feb 12, 2022

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Oct 11, 2021

Our current Factorization structures often hard-code the storage of indices, coefficients, ... resulting in downstream packages like CUDA.jl or StaticArrays.jl having to re-implement these structures for compatibility. For example, https://github.com/JuliaArrays/StaticArrays.jl/blob/559134c9edda3b8ee9a71938386d2904338d7cc9/src/qr.jl#L1-L6 or https://github.com/JuliaGPU/CUDA.jl/blob/43e0f48b135325b97415cd1c8802e47732de85c6/lib/cusolver/linalg.jl#L77-L81. This is unfortunate, since it requires duplicating a lot of functionality, and requires special-casing in packages like ChainRules.jl.

This PR explores adding additional type parameters to a couple of factorization structures (more to be added) to explore the fallout such a change. It's unlikely that we can do this in Julia 1.x, but maybe PkgEval tells otherwise...

cc @andreasnoack, ref JuliaGPU/CUDA.jl#1194 (comment).

@maleadt maleadt added speculative Whether the change will be implemented is speculative linear algebra Linear algebra labels Oct 11, 2021
@timholy
Copy link
Member

timholy commented Oct 11, 2021

This will also be nice for the day when we finally get around to implementing arbitrary-axis support for appropriate parts of linear algebra.

@maleadt
Copy link
Member Author

maleadt commented Oct 11, 2021

CI seems OK with this, so let's try PkgEval:

@nanosoldier runbenchmarks(ALL, vs=":master")

Note that I haven't updated all factorizations, so this may result in too positive of a report.

@nanosoldier

This comment has been minimized.

@maleadt
Copy link
Member Author

maleadt commented Oct 11, 2021

@nanosoldier runtests(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@andreasnoack
Copy link
Member

None of the test errors are related to this change so it might actually be fine go move forward with this in 1.x

@maleadt
Copy link
Member Author

maleadt commented Oct 12, 2021

None of the test errors are related to this change so it might actually be fine go move forward with this in 1.x

That's encouraging. I've updated the PR to include the remaining factorizations; I'll do another PkgEval run when CI passes.

@dkarrasch
Copy link
Member

I wouldn't have expected errors, actually. I think the one major concern about adding type parameters is that when these are types of fields, the formely concrete types become unsufficiently parametrized and hence abstract, which may affect performance. But I'm not proficient enough with our package search engine to find out if that's a common pattern in the ecosystem.

@maleadt
Copy link
Member Author

maleadt commented Oct 12, 2021

@nanosoldier runtests(ALL, vs = ":master")

@fredrikekre
Copy link
Member

I wouldn't have expected errors, actually. I think the one major concern about adding type parameters is that when these are types of fields, the formely concrete types become unsufficiently parametrized and hence abstract, which may affect performance. But I'm not proficient enough with our package search engine to find out if that's a common pattern in the ecosystem.

Yes, I think we have avoided adding extra type parameters for this reason in the past.

@maleadt
Copy link
Member Author

maleadt commented Oct 12, 2021

I wouldn't expect there to be many structs capturing factorizations, but there exist some at least. From a quick look at JuliaHub, filtering out instances that are currently abstract already:

I'd be happy to take a closer look and submit PRs to packages if that's an argument in favor.

@andreasnoack
Copy link
Member

I think this change is worth it even if some structs become weakly typed. This is something that was requested before 1.0 but we didn't get around to so if it can be done without breaking code then I think we should move forward.

@timholy
Copy link
Member

timholy commented Oct 12, 2021

We added a type-parameter to ReinterpretArray that allowed us to get a 20x performance boost in some benchmarks (the new reinterpret(reshape, ...) framework), and StepRangeLen recently added one for correctness to allow the length to be encoded by something other than Int. I know the first triggered updates in some packages but there were, if memory serves, relatively few vocal complaints.

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@maleadt
Copy link
Member Author

maleadt commented Feb 9, 2022

Rebased. Let's do another PkgEval run to see if there isn't any unexpected breakage since last time:

@nanosoldier runtests(ALL, vs = ":master")

With the 1.8 feature freeze soon, do we want to consider this? Comments above seemed positive, PkgEval fallout minimal, and for the few packages that do regress (in terms of functionality of performance) I can create PRs to fix them.

@vtjnash
Copy link
Member

vtjnash commented Feb 9, 2022

SGTM

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@maleadt
Copy link
Member Author

maleadt commented Feb 10, 2022

@nanosoldier runtests(["CompressedSensing", "Conductor", "CopEnt", "DiffEqOperators", "DiffEqParamEstim", "DiscreteValueIteration", "DynamicAxisWarping", "ECharts", "Evolutionary", "GaussianMixtureAlignment", "HomotopyContinuation", "HypercubeTransform", "ImmersedLayers", "MRIReco", "MatrixPencils", "NavAbilitySDK", "NumericalAlgorithms", "ParetoSmooth", "Pathfinder", "Polylogarithms", "PowerSystems", "ReactionMechanismSimulator", "SignalAnalysis", "SimpleWorkflows", "Spirograph", "StatsFuns", "StochasticGene", "Sundials", "SymFEL", "VoronoiGraph"], vs = ":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@maleadt
Copy link
Member Author

maleadt commented Feb 11, 2022

Package test failures are related, but respectively in @allocated and @inferred tests, so it seems nonbreaking:

HomotopyContinuation:

┌ Warning: `LU{T, S}(factors::AbstractMatrix{T}, ipiv::AbstractVector{<:Integer}, info::BlasInt) where {T, S}` is deprecated, use `LU{T, S, typeof(ipiv)}(factors, ipiv, info)` instead.
│   caller = ip:0x0
└ @ Core :-1
ldiv: Test Failed at /home/pkgeval/.julia/packages/HomotopyContinuation/UFWkw/test/linear_algebra_test.jl:60
  Expression: #= /home/pkgeval/.julia/packages/HomotopyContinuation/UFWkw/test/linear_algebra_test.jl:60 =# @allocated(ldiv!(x, WS, b)) == 0
   Evaluated: 32 == 0

Pathfinder:

A diag, D diag: Error During Test at /home/pkgeval/.julia/packages/Pathfinder/7iiV9/test/woodbury.jl:32
  Got exception outside of a @test
  return type WoodburyPDMat{Float32, Diagonal{Float32, Vector{Float32}}, Matrix{Float32}, Diagonal{Float32, Vector{Float32}}, Diagonal{Float32, Vector{Float32}}, QRCompactWYQ{Float32, Matrix{Float32}, Matrix{Float32}}, UpperTriangular{Float32, Matrix{Float32}}} does not match inferred return type Union{WoodburyPDMat{Float32, Diagonal{Float32, Vector{Float32}}, Matrix{Float32}, Diagonal{Float32, Vector{Float32}}, Diagonal{Float32, Vector{Float32}}, QRCompactWYQ{Float32, Matrix{Float32}, Matrix{Float32}}, UpperTriangular{Float32, Matrix{Float32}}}, WoodburyPDMat{Float32, Diagonal{Float32, Vector{Float32}}, Matrix{Float32}, Diagonal{Float32, Vector{Float32}}, Diagonal{Float32, Vector{Float32}}, QRCompactWYQ{Float32, Matrix{Float32}, QRCompactWYQ{Float32, Matrix{Float32}, Matrix{Float32}}}, UpperTriangular{Float32, Matrix{Float32}}}}

Some packages generate warnings, but don't cause test failures:

WARNING: Method definition logabsdet(Union{LinearAlgebra.Cholesky{T, S} where S<:(AbstractArray{T, 2} where T) where T, LinearAlgebra.CholeskyPivoted{T, S, P} where P<:(AbstractArray{var"#s879", 1} where var"#s879"<:Integer) where S<:(AbstractArray{T, 2} where T) where T}) in module LinearAlgebra at /workspace/srcdir/usr/share/julia/stdlib/v1.8/LinearAlgebra/src/cholesky.jl:695 overwritten in module WoodburyFactorizations at /home/pkgeval/.julia/packages/WoodburyFactorizations/zmYkv/src/WoodburyFactorizations.jl:146.

So all in, this looks reasonable, and I think we can merge this for 1.8. I'll deal with the fallout, and once it's merged look at fixing or improving packages.

@dkarrasch
Copy link
Member

The ambiguity in Pathfinder.jl is due to the fact that type inference believes there can be a QRCompactWYQ{Float32, Matrix{Float32}, QRCompactWYQ{Float32, Matrix{Float32}, Matrix{Float32}}}, which may indicate that there is some no-op constructor missing, but looks fixable, I agree.

Introduce additional type parameters to remove hard-coded arrays from structures.
@maleadt
Copy link
Member Author

maleadt commented Feb 11, 2022

Another rebase to confirm CI failures are unrelated (tester_linux64 had a linalg failure, but the log was not clear).
Should be good to go once that passes.

@maleadt maleadt added merge me PR is reviewed. Merge when all tests are passing and removed speculative Whether the change will be implemented is speculative labels Feb 11, 2022
@maleadt maleadt changed the title RFC: Generalize storage of factorizations. Generalize storage of factorizations. Feb 11, 2022
@maleadt
Copy link
Member Author

maleadt commented Feb 12, 2022

GC failure is known issue with ASAN after the recent effects PR, so this looks good to go.

@maleadt maleadt merged commit 7826ac8 into master Feb 12, 2022
@maleadt maleadt deleted the tb/factorizations branch February 12, 2022 17:46
@maleadt maleadt removed the merge me PR is reviewed. Merge when all tests are passing label Feb 12, 2022
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request Feb 17, 2022
Introduce additional type parameters to remove hard-coded arrays from structures.
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
Introduce additional type parameters to remove hard-coded arrays from structures.
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
Introduce additional type parameters to remove hard-coded arrays from structures.
vtjnash pushed a commit that referenced this pull request Jul 5, 2022
KristofferC pushed a commit that referenced this pull request Jul 6, 2022
ffucci pushed a commit to ffucci/julia that referenced this pull request Aug 11, 2022
pcjentsch pushed a commit to pcjentsch/julia that referenced this pull request Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants