-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Base.LinAlg to LinearAlgebra stdlib #25571
Base.LinAlg to LinearAlgebra stdlib #25571
Conversation
very exciting! |
Thank you! |
|
3545b04
to
d2dbcce
Compare
Small thing but should remove the |
A nice thing Fredrik told me about is the CI time. Before this PR, the linalg tests (on the freebsd builder) took a total of 3580 seconds. WIth all the linalg running on the same worker, it now takes 1842 seconds. |
base/exports.jl
Outdated
# vecdot, | ||
# vecnorm, | ||
# ⋅, | ||
# ×, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these should just be deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of the working process, will clean up the PR :)
base/interactiveutil.jl
Outdated
println(io, " BLAS: ",libblas_name) | ||
end | ||
println(io, " LAPACK: ",liblapack_name) | ||
# if Base.libblas_name == "libopenblas" || BLAS.vendor() == :openblas || BLAS.vendor() == :openblas64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to remove this code, it should also just be deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, will add this as a LinearAlgebra.versioninfo()
function for now.
I think I should wait for this before merging #25421 |
Has there been any thought of about the fact that stdlibs still "affect" Base functions by using method extension and being in the sysimg? For example, it is likely that there is still something in Base using matrix multiplication which now only works because LinearAlgebra is in the sysimg. |
My thought on that is we will try to make them less coupled over time, and a change like this is a step in that direction. |
Eventually it'd be nice to have some kind of compile flag to say |
I think that not all linear algebra definitions can be moved out, but that's ok. The line ends up being that we need in-Base implementations when the operation and its argument types all belong to Base – i.e. precisely when LinAlg would be committing type piracy (second degree, not first). Matmul is a good example. But methods on types that don't belong to Base and operations that aren't defined in Base can live entirely in LinAlg. We could even have a pure Julia definition of matmul in Base and then overwrite it with a definition that calls BLAS in LinAlg. So there are definitely options here. |
What are first / second degree type piracy? |
Terms I just made up 😬 ... but which I define as for function and args types you don't own:
So first degree is a form of monkey patching and can have non-local effects on program behavior. Second degree is not monkey patching and is far less risky but could still have a non-local effect on behavior if some other code overwrites the same new methods that aren't theirs. |
I think stdlibs should have the licence to commit second degree type piracy 😄 |
Agreed. |
8b73a98
to
e2fc62b
Compare
@@ -391,7 +391,12 @@ end | |||
function _one(unit::T, x::AbstractMatrix) where T | |||
m,n = size(x) | |||
m==n || throw(DimensionMismatch("multiplicative identity defined only for square matrices")) | |||
Matrix{T}(I, m, m) | |||
# Matrix{T}(I, m, m) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function should probably be moved too, but can stay for now.
@@ -506,7 +506,7 @@ end | |||
|
|||
Compute the hypotenuse ``\\sqrt{\\sum x_i^2}`` avoiding overflow and underflow. | |||
""" | |||
hypot(x::Number...) = vecnorm(x) | |||
hypot(x::Number...) = sqrt(sum(abs2(y) for y in x)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of two places which uses a LinearAlgebra
owned function... I hope this hack is fine for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should consider deprecating this function or just move it since the raison d'être of hypot
is to handle potential overflow which vecnorm
does but this implementation doesn't
@@ -328,7 +330,7 @@ unscaled_covzm(x::AbstractVector{<:Number}) = sum(abs2, x) | |||
unscaled_covzm(x::AbstractVector) = sum(t -> t*t', x) | |||
unscaled_covzm(x::AbstractMatrix, vardim::Int) = (vardim == 1 ? _conj(x'x) : x * x') | |||
|
|||
unscaled_covzm(x::AbstractVector, y::AbstractVector) = dot(y, x) | |||
unscaled_covzm(x::AbstractVector, y::AbstractVector) = sum(conj(y[i])*x[i] for i in eachindex(y, x)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second place where a LinearAlgebra
owned function is used. This should be ok for now...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not equivalent for all element types. Should we move statistics to stdlib first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This indeed turned out to be a problem in some cases. See https://github.com/JuliaLang/Statistics.jl/pull/85.
@@ -59,13 +59,13 @@ julia> A = [1 2; 2 3] | |||
2 3 | |||
|
|||
julia> bkfact(A) | |||
Base.LinAlg.BunchKaufman{Float64,Array{Float64,2}} | |||
LinearAlgebra.BunchKaufman{Float64,Array{Float64,2}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is much prettier IMO, rather awkward with the Base.
prefix before 🙂
e2fc62b
to
b40d312
Compare
9e37ae2
to
a056c2f
Compare
Looking good! Ready to merge? |
I think so. Since CI pass it should be fine. Can always fix up stuff in subsequent PRs. @Sacha0 ? |
If @fredrikekre is happy, I am happy :). Otherwise I will continue reviewing this over the next few days. Best! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pretty big diff but I think it mostly fine. I only have a few comments.
@@ -70,7 +60,6 @@ export | |||
IOBuffer, | |||
IOStream, | |||
LinSpace, | |||
LowerTriangular, | |||
Irrational, | |||
Matrix, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we want to define these aliases in LinearAlgebra
@@ -506,7 +506,7 @@ end | |||
|
|||
Compute the hypotenuse ``\\sqrt{\\sum x_i^2}`` avoiding overflow and underflow. | |||
""" | |||
hypot(x::Number...) = vecnorm(x) | |||
hypot(x::Number...) = sqrt(sum(abs2(y) for y in x)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should consider deprecating this function or just move it since the raison d'être of hypot
is to handle potential overflow which vecnorm
does but this implementation doesn't
@@ -59,7 +59,8 @@ julia> mean!([1. 1.], v) | |||
""" | |||
function mean!(R::AbstractArray, A::AbstractArray) | |||
sum!(R, A; init=true) | |||
scale!(R, max(1, _length(R)) // _length(A)) | |||
x = max(1, _length(R)) // _length(A) | |||
R .= R .* x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this could R .*= x
@@ -328,7 +330,7 @@ unscaled_covzm(x::AbstractVector{<:Number}) = sum(abs2, x) | |||
unscaled_covzm(x::AbstractVector) = sum(t -> t*t', x) | |||
unscaled_covzm(x::AbstractMatrix, vardim::Int) = (vardim == 1 ? _conj(x'x) : x * x') | |||
|
|||
unscaled_covzm(x::AbstractVector, y::AbstractVector) = dot(y, x) | |||
unscaled_covzm(x::AbstractVector, y::AbstractVector) = sum(conj(y[i])*x[i] for i in eachindex(y, x)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not equivalent for all element types. Should we move statistics to stdlib first?
🎉 |
This is awesome! Is the deprecation for |
this commit removes cor, cov, median, median!, middle, quantile, quantile!, std, stdm, var, varm and linreg and moves them to StatsBase fix #25571 (comment) (included in StatsBase.jl/#379) fix #23769 (included in StatsBase.jl/#379) fix #27140
this commit removes cor, cov, median, median!, middle, quantile, quantile!, std, stdm, var, varm and linreg and moves them to StatsBase fix #25571 (comment) (included in JuliaStats/StatsBase.jl#379) fix #23769 (included in JuliaStats/StatsBase.jl#379) fix #27140
fix #25571 (comment) (included in JuliaStats/StatsBase.jl#379) fix #23769 (included in JuliaStats/StatsBase.jl#379) fix #27140
fix #25571 (comment) (included in JuliaStats/StatsBase.jl#379) fix #23769 (included in JuliaStats/StatsBase.jl#379) fix #27140
fix #25571 (comment) (included in JuliaStats/StatsBase.jl#379) fix #23769 (included in JuliaStats/StatsBase.jl#379) fix #27140
fix #25571 (comment) (included in JuliaStats/StatsBase.jl#379) fix #23769 (included in JuliaStats/StatsBase.jl#379) fix #27140
fix #25571 (comment) (included in JuliaStats/StatsBase.jl#379) fix #23769 (included in JuliaStats/StatsBase.jl#379) fix #27140
fix JuliaLang/julia#25571 (comment) (included in JuliaStats/StatsBase.jl#379) fix #23769 (included in JuliaStats/StatsBase.jl#379) fix #27140
fix JuliaLang/julia#25571 (comment) (included in JuliaStats/StatsBase.jl#379) fix #23769 (included in JuliaStats/StatsBase.jl#379) fix #27140
fix JuliaLang/julia#25571 (comment) (included in JuliaStats/StatsBase.jl#379) fix #23769 (included in JuliaStats/StatsBase.jl#379) fix #27140
fix JuliaLang/julia#25571 (comment) (included in JuliaStats/StatsBase.jl#379) fix #23769 (included in JuliaStats/StatsBase.jl#379) fix #27140
Move
Base.LinAlg
to the newLinearAlgebra
standard library module.TODO:
@deprecate_moved
everything