Skip to content

WIP: get rid of A*_mul_B! #74

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

Closed
wants to merge 10 commits into from
Closed

WIP: get rid of A*_mul_B! #74

wants to merge 10 commits into from

Conversation

dkarrasch
Copy link
Member

@dkarrasch dkarrasch commented Nov 18, 2019

This started innocently, and became a major code overhaul. I realized somewhere in #72 I guess that one can get rid of all the A*_mul_B! by dispatchingmul! on things like AdjointMap{<:Any,<:SomeLinearMap}. This turned out to be really nice, because for subtypes of LinearMaps, that are invariant under transposition/adjoint, certain combinations are now impossible to obtain by standard means, even when artificially wrapping, say, a CompositeMap in a LinearMap and only then taking the tranpose/adjoint. The beauty of it is that, eventually, up to some helper functions, this package is back where at was pre-0.7, that is, provides types and function overloads for Base or LinearAlgebra functions (with kronsum and some symbols the only exceptions).

For review, I suggest to skip blockmap.jl, because that may require a rebase after #72 is merged (even though I simplified some code already).

EDIT: I forgot to mention that this also takes big steps forward in providing specialized 5-arg mul!s for both vectors and matrices.

@coveralls
Copy link

coveralls commented Nov 18, 2019

Coverage Status

Coverage decreased (-6.6%) to 86.477% when pulling e948e23 on bounds into 9f2d89d on master.

@codecov
Copy link

codecov bot commented Nov 18, 2019

Codecov Report

Merging #74 into master will decrease coverage by 10.03%.
The diff coverage is 66.06%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #74       +/-   ##
===========================================
- Coverage   98.50%   88.47%   -10.04%     
===========================================
  Files          10       10               
  Lines         738      651       -87     
===========================================
- Hits          727      576      -151     
- Misses         11       75       +64     
Impacted Files Coverage Δ
src/blockmap.jl 77.10% <29.41%> (-20.71%) ⬇️
src/kronecker.jl 89.18% <42.85%> (-10.82%) ⬇️
src/composition.jl 90.00% <57.14%> (-8.62%) ⬇️
src/wrappedmap.jl 94.28% <88.88%> (-5.72%) ⬇️
src/LinearMaps.jl 88.57% <100.00%> (-9.92%) ⬇️
src/functionmap.jl 97.36% <100.00%> (-0.39%) ⬇️
src/linearcombination.jl 100.00% <100.00%> (ø)
src/transpose.jl 88.46% <100.00%> (-4.40%) ⬇️
src/uniformscalingmap.jl 95.91% <100.00%> (-2.23%) ⬇️
... and 8 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 e30dac8...6407d35. Read the comment docs.

@dkarrasch
Copy link
Member Author

Currently, tests are failing in the linearcombinations tests, where a single allocation counting test fails. I can't reproduce this locally (on macos), tests pass all fine here on all available Julia versions. Would be great if somebody could check out this branch and see if that is spurious. I did notice though that 5-arg mul! with matrices is certainly not allocation-free on Julia v1.3.rc's.

using LinearAlgebra, BenchmarkTools
A = rand(100,200)
x = rand(size(A, 2), 10)
y = rand(size(A, 1), 10)
α = rand()
β = rand()
@benchmark mul!($y, $A, $x, $α, $β)

BenchmarkTools.Trial: 
  memory estimate:  64 bytes
  allocs estimate:  3
  --------------
  minimum time:     19.190 μs (0.00% GC)
  median time:      20.584 μs (0.00% GC)
  mean time:        24.629 μs (0.00% GC)
  maximum time:     475.044 μs (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     1

And it does seem to cost performance. I assume this is due to the generation of the MulAdd structure. At least, on Julia v1.4, these are gone (again, locally), and the above median time is significantly lower!

@dkarrasch dkarrasch changed the title get rid of A*_mul_B! WIP: get rid of A*_mul_B! Nov 18, 2019
@dkarrasch dkarrasch mentioned this pull request Nov 20, 2019
@dkarrasch
Copy link
Member Author

This PR requires a little work to merge the other PRs in, e.g., the mulstyle trait. When those are merged, I'll finalize this one and shout again for review.

@JeffFessler
Copy link
Member

I hope to return to #65 (IndexMap) after this simplification process is finalized.

@dkarrasch
Copy link
Member Author

This is totally outdated. I took some of the benchmarks and put them in #116. The final thing that this PR tried to address is to allocate only a single intermediate vector when doing 5-arg map-matrix multiplication for a ThreeArg mulstyle. But that requires a fresh approach, perhaps.

@dkarrasch dkarrasch closed this Oct 9, 2020
@dkarrasch dkarrasch deleted the bounds branch October 9, 2020 09:12
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