Base.LinAlg to LinearAlgebra stdlib #25571

fredrikekre · 2018-01-15T16:25:49Z

Move Base.LinAlg to the new LinearAlgebra standard library module.

TODO:

Hunt down the last ~10 test failures
Fix documentations
@deprecate_moved everything

quinnj · 2018-01-15T17:38:29Z

very exciting!

ViralBShah · 2018-01-15T18:30:39Z

Thank you!

fredrikekre · 2018-01-17T09:39:01Z

Test Summary: |     Pass  Broken     Total
  Overall     | 26598697  328325  26927022
    SUCCESS

KristofferC · 2018-01-17T16:02:35Z

Small thing but should remove the linalg test group from test/Makefile.

KristofferC · 2018-01-17T16:42:52Z

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.

JeffBezanson · 2018-01-17T20:19:05Z

base/exports.jl

+#     vecdot,
+#     vecnorm,
+#     ⋅,
+#     ×,


All these should just be deleted.

Part of the working process, will clean up the PR :)

JeffBezanson · 2018-01-17T20:19:21Z

base/interactiveutil.jl

-        println(io, "  BLAS: ",libblas_name)
-    end
-    println(io, "  LAPACK: ",liblapack_name)
+    # if Base.libblas_name == "libopenblas" || BLAS.vendor() == :openblas || BLAS.vendor() == :openblas64


If we're going to remove this code, it should also just be deleted.

Yes, will add this as a LinearAlgebra.versioninfo() function for now.

JeffBezanson · 2018-01-17T20:27:55Z

I think I should wait for this before merging #25421

KristofferC · 2018-01-17T20:30:21Z

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.

JeffBezanson · 2018-01-17T20:33:47Z

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.

quinnj · 2018-01-17T20:44:57Z

Eventually it'd be nice to have some kind of compile flag to say --NO_LINALG to avoid including the LinAlg module or any of it's source dependencies. It would then be pretty clear that there are no dependencies in Base (or maybe you'd just get a MethodError).

StefanKarpinski · 2018-01-17T21:12:35Z

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.

KristofferC · 2018-01-17T21:19:39Z

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)

What are first / second degree type piracy?

StefanKarpinski · 2018-01-17T21:59:22Z

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:

"first degree type piracy" = redefining existing methods that aren't yours
"second degree type piracy" = defining new methods that aren't yours

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.

fredrikekre · 2018-01-17T22:01:34Z

I think stdlibs should have the licence to commit second degree type piracy 😄

JeffBezanson · 2018-01-17T22:05:58Z

Agreed.

StefanKarpinski · 2018-01-17T22:21:03Z

fredrikekre · 2018-01-17T23:48:31Z

base/array.jl

@@ -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)


This function should probably be moved too, but can stay for now.

fredrikekre · 2018-01-17T23:49:09Z

base/math.jl

@@ -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))


One of two places which uses a LinearAlgebra owned function... I hope this hack is fine for now.

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

fredrikekre · 2018-01-17T23:49:44Z

base/statistics.jl

@@ -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))


The second place where a LinearAlgebra owned function is used. This should be ok for now...

This is not equivalent for all element types. Should we move statistics to stdlib first?

This indeed turned out to be a problem in some cases. See https://github.com/JuliaLang/Statistics.jl/pull/85.

fredrikekre · 2018-01-17T23:52:34Z

stdlib/LinearAlgebra/src/bunchkaufman.jl

@@ -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}}


This is much prettier IMO, rather awkward with the Base. prefix before 🙂

JeffBezanson · 2018-01-18T19:02:07Z

Looking good! Ready to merge?

fredrikekre · 2018-01-18T19:41:38Z

Looking good! Ready to merge?

I think so. Since CI pass it should be fine. Can always fix up stuff in subsequent PRs. @Sacha0 ?

Sacha0 · 2018-01-18T20:00:38Z

If @fredrikekre is happy, I am happy :). Otherwise I will continue reviewing this over the next few days. Best!

andreasnoack

This is a pretty big diff but I think it mostly fine. I only have a few comments.

andreasnoack · 2018-01-18T19:12:41Z

base/exports.jl

@@ -70,7 +60,6 @@ export
    IOBuffer,
    IOStream,
    LinSpace,
-    LowerTriangular,
    Irrational,
    Matrix,


Maybe we want to define these aliases in LinearAlgebra

andreasnoack · 2018-01-18T19:19:40Z

base/math.jl

@@ -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))


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

andreasnoack · 2018-01-18T20:17:32Z

base/statistics.jl

@@ -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


I believe this could R .*= x

andreasnoack · 2018-01-18T20:48:40Z

base/statistics.jl

@@ -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))


This is not equivalent for all element types. Should we move statistics to stdlib first?

JeffBezanson · 2018-01-18T20:58:56Z

🎉

Sacha0 · 2018-01-18T21:02:53Z

mbauman · 2018-01-20T01:45:56Z

This is awesome!

Is the deprecation for I missing?

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 JuliaLang/julia#25571 (comment) (included in JuliaStats/StatsBase.jl#379) fix #23769 (included in JuliaStats/StatsBase.jl#379) fix #27140

fredrikekre added domain:linear algebra Linear algebra kind:excision Removal of code from Base or the repository stdlib Julia's standard library labels Jan 15, 2018

JeffBezanson added this to the 1.0 milestone Jan 17, 2018

fredrikekre force-pushed the so-long-and-thanks-for-all-the-fish branch from 3545b04 to d2dbcce Compare January 17, 2018 11:21

JeffBezanson reviewed Jan 17, 2018

View reviewed changes

fredrikekre force-pushed the so-long-and-thanks-for-all-the-fish branch 2 times, most recently from 8b73a98 to e2fc62b Compare January 17, 2018 23:47

fredrikekre commented Jan 17, 2018

View reviewed changes

fredrikekre changed the title ~~wip linalg stdlib~~ linalg stdlib Jan 17, 2018

fredrikekre changed the title ~~linalg stdlib~~ Base.LinAlg to LinearAlgebra stdlib Jan 17, 2018

fredrikekre requested review from andreasnoack and Sacha0 January 17, 2018 23:54

fredrikekre force-pushed the so-long-and-thanks-for-all-the-fish branch from e2fc62b to b40d312 Compare January 18, 2018 00:04

move Base.LinAlg to LinearAlgebra stdlib

a056c2f

fredrikekre force-pushed the so-long-and-thanks-for-all-the-fish branch from 9e37ae2 to a056c2f Compare January 18, 2018 13:57

andreasnoack reviewed Jan 18, 2018

View reviewed changes

Sacha0 merged commit 7deac81 into JuliaLang:master Jan 18, 2018

fredrikekre deleted the so-long-and-thanks-for-all-the-fish branch January 18, 2018 20:59

fredrikekre mentioned this pull request Jan 21, 2018

fix deprecation of I #25666

Merged

Enet4 mentioned this pull request Jan 22, 2018

Include LinearAlgebra module in micro benchmark #25686

Merged

KristofferC mentioned this pull request Jan 27, 2018

allow stdlibs to run their tests in parallel #25778

Merged

fredrikekre mentioned this pull request Mar 10, 2018

add modphase keyword for isapprox, deprecate @test_approx_eq_modphase #20021

Closed

cormullion mentioned this pull request Mar 18, 2018

NEWS.md is getting a bit untidy #26508

Closed

giordano mentioned this pull request May 17, 2018

Vararg version of hypot does overflow and underflow on master #27141

Closed

fredrikekre added a commit that referenced this pull request May 25, 2018

move cor, cov, std, stdm, var, varm and linreg to StatsBase

1aa4c99

fix #25571 (comment) (included in JuliaStats/StatsBase.jl#379) fix #23769 (included in JuliaStats/StatsBase.jl#379) fix #27140

fredrikekre added a commit that referenced this pull request May 25, 2018

move cor, cov, std, stdm, var, varm and linreg to StatsBase

f27a71d

fix #25571 (comment) (included in JuliaStats/StatsBase.jl#379) fix #23769 (included in JuliaStats/StatsBase.jl#379) fix #27140

fredrikekre added a commit that referenced this pull request May 25, 2018

move cor, cov, std, stdm, var, varm and linreg to StatsBase

fe2b611

fix #25571 (comment) (included in JuliaStats/StatsBase.jl#379) fix #23769 (included in JuliaStats/StatsBase.jl#379) fix #27140

fredrikekre added a commit that referenced this pull request May 28, 2018

move cor, cov, std, stdm, var, varm and linreg to StatsBase

46a477a

fix #25571 (comment) (included in JuliaStats/StatsBase.jl#379) fix #23769 (included in JuliaStats/StatsBase.jl#379) fix #27140

ViralBShah pushed a commit that referenced this pull request May 28, 2018

move cor, cov, std, stdm, var, varm and linreg to StatsBase (#27152)

746d08f

fix #25571 (comment) (included in JuliaStats/StatsBase.jl#379) fix #23769 (included in JuliaStats/StatsBase.jl#379) fix #27140

giordano mentioned this pull request May 6, 2019

hypot with complex arguments #31941

Closed

nalimilan mentioned this pull request Sep 13, 2021

Improve numerical stability of cov JuliaStats/Statistics.jl#85

Merged

Base.LinAlg to LinearAlgebra stdlib #25571

Base.LinAlg to LinearAlgebra stdlib #25571

Conversation

fredrikekre commented Jan 15, 2018 • edited Loading

quinnj commented Jan 15, 2018

ViralBShah commented Jan 15, 2018

fredrikekre commented Jan 17, 2018

KristofferC commented Jan 17, 2018

KristofferC commented Jan 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffBezanson commented Jan 17, 2018

KristofferC commented Jan 17, 2018

JeffBezanson commented Jan 17, 2018

quinnj commented Jan 17, 2018

StefanKarpinski commented Jan 17, 2018

KristofferC commented Jan 17, 2018 • edited Loading

StefanKarpinski commented Jan 17, 2018 • edited Loading

fredrikekre commented Jan 17, 2018

JeffBezanson commented Jan 17, 2018

StefanKarpinski commented Jan 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffBezanson commented Jan 18, 2018

fredrikekre commented Jan 18, 2018

Sacha0 commented Jan 18, 2018

andreasnoack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffBezanson commented Jan 18, 2018

Sacha0 commented Jan 18, 2018

mbauman commented Jan 20, 2018

fredrikekre commented Jan 15, 2018 •

edited

Loading

KristofferC commented Jan 17, 2018 •

edited

Loading

StefanKarpinski commented Jan 17, 2018 •

edited

Loading