Skip to content

Conversation

dlfivefifty
Copy link
Member

@dlfivefifty dlfivefifty commented Aug 22, 2018

This PR does the following:

  1. Drops support for Julia 0.6
  2. Uses LazyArrays.jl to simplify BLAS-like linear algebra, for example, the following lowers to a BLAS gbmv! call:
A = brand(1000,1000,1,1)
x = randn(1000)
y = similar(x)
y .= 2.0 .* Mul(A,x) .+ 3.0 .* y
  1. Drops the interface macros. Something that conforms to the banded matrix interface needs to now override MemoryLayout to return an AbstractBandedLayout, and only supports LazyArrays.jl syntax for fast banded linear algebra: LinearAlgebra.mul! and Base.* will not automatically call banded routines. This is because they require too many ambiguity overrides, whereas LazyArrays.jl uses traits.
  2. No longer calls gbmm! for Banded * Banded. This is temporary: that needs to be rewritten to call BLAS Level 3 to get the desired speed.
  3. Resolves Make Adjoint{T,<:BandedMatrix}, conform to banded matrix interface #48

@MikaelSlevinsky If you have any comments Re our discussion. Once this is merged I'll add support triangular banded matrices. I will also remove SymBandedMatrix in favour of Symmetric{BandedMatrix}.

@randy3k This is a substantial departure from the linear algebra code you wrote. I believe this is necessary to reduce code complexity and simplify use in BlockBandedMatrices.jl (where the blocks of BandedBlockBandedMatrix conform to the banded matrix interface). But any suggestions will be taken on board.

@dlfivefifty
Copy link
Member Author

It requires LazyArrays.jl master for now.

@MikaelSlevinsky
Copy link
Member

Turns out there is partial support for triangular banded through BLAS (rather than LAPACK): actually, I'm using this function tbsv! as is, i.e. with no dimension/error checking.

This feels like a rather major change. Are other matrix packages moving away from overriding mul!, * and other Base functions?

@dlfivefifty
Copy link
Member Author

What other packages support views conforming to the same interface?

The change is also motivated by the PR JuliaLang/julia#25558 which was close to being merged into Base, but then I decided a lazy Mul was a better approach and it could be developed for now in its own package. So the eventual goal is that LazyArrays.jl's approach (or something similar) replaces mul! in LinearAlgebra.jl.

Note that anything <: AbstractBandedMatrix still supports Base routines.

@MikaelSlevinsky
Copy link
Member

In some instances, the view of a low rank matrix, or a Toeplitz matrix could also support the same interface. I don't think those packages have interfaces at this point if that's what you're asking.

@dlfivefifty
Copy link
Member Author

The nice thing about avoiding Base is support for LazyArrays.jl can be added to existing matrix types without any changes to the package. So one could add support for

   T = Toeplitz(...)
   V = view(T,1:5,1:5)
   x .= Mul(V, y)

without having to make any PR into ToeplitzMatrices.jl, and with no type piracy.

@MikaelSlevinsky
Copy link
Member

Shouldn't LazyArrays behaviour be in stdlib first? Or is this PR part of the proof-of-concept?

@dlfivefifty
Copy link
Member Author

Why? What benefit does it get from being in StdLib? By that logic, FillArrays should also not be used until it’s in Base. Shouldn’t BandedMatrix be in StdLib too before it’s used?

The standard for getting code in Base/StdLib is higher than in user packages. It follows that this kind of change will be in packages before it is in StdLib, not the other way around.

@MikaelSlevinsky
Copy link
Member

I get experimenting before contributing to stdlib, but why have a stdlib that's too burdensome to use? (mul! et al.)

@dlfivefifty
Copy link
Member Author

Because we couldn’t agree to what MemoryLayouts should be included, only a few packages are impacted, and it wasn’t worth delaying 1.0 over. @mbauman has said he’ll look at that PR again in the future.

In the meantime, this gives me the functionality I need while also serving as a proof of concept. Also, this is all hidden from users, and is no more bizarre than what was already there.

Also, I need LazyArrays to support the syntax BandedMatrix(Kron(A,B)) and BandedBlockBandedMatrix(Kron(A,B)). The latter is particularly useful for distributed arrays, where the lazy Kron can be cheaply communicated to workers.

@codecov-io
Copy link

codecov-io commented Aug 27, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@8b51afc). Click here to learn what that means.
The diff coverage is 90.28%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #67   +/-   ##
=========================================
  Coverage          ?   93.26%           
=========================================
  Files             ?       16           
  Lines             ?      594           
  Branches          ?        0           
=========================================
  Hits              ?      554           
  Misses            ?       40           
  Partials          ?        0
Impacted Files Coverage Δ
src/banded/gbmm.jl 95.23% <ø> (ø)
src/lapack.jl 95.45% <ø> (ø)
src/blas.jl 100% <ø> (ø)
src/banded/BandedLU.jl 100% <100%> (ø)
src/symbanded/SymBandedMatrix.jl 96.29% <100%> (ø)
src/banded/BandedMatrix.jl 92.95% <100%> (ø)
src/generic/interface.jl 100% <100%> (ø)
src/banded/linalg.jl 100% <100%> (ø)
src/generic/AbstractBandedMatrix.jl 92.85% <100%> (ø)
src/banded/BandedQR.jl 96.92% <100%> (ø)
... and 4 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 8b51afc...9b7a7e3. Read the comment docs.

@dlfivefifty dlfivefifty merged commit 077a55f into master Aug 27, 2018
@dlfivefifty dlfivefifty deleted the dl/lazyarrays branch August 28, 2018 10:15
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.

3 participants