add mulstyle feature

[dkarrasch]

Closes #75. @jagot

This adds the mulstyle trait to LinearMaps.jl and some tests.

[dkarrasch]

@Jutho If you find the time for review, I'd say this has highest priority. The next one is #72. 😉

[jagot]

I think this looks really nice!

One could think of replacing
https://github.com/Jutho/LinearMaps.jl/blob/e2cb3a13fc4d2dac9e7f6f78d0a2bee66cccb91f/src/linearcombination.jl#L104-L110

with something like

An = A.maps[n]
muladd!(mulstyle(An), y, An, x, z)

and then introduce the functions

muladd!(::FiveArg, y, A, x, _) = mul!(y, A, x, true, true)
function muladd!(::ThreeArg, y, A, x, z)
    A_mul_B!(z, A, x) 
    y .+= z
end

but that is stylish nit-picking.

[coveralls]

Coverage increased (+1.1%) to 94.284% when pulling 7cd8c98 on mulstyle into dd960a5 on master.

[codecov]

Codecov Report

Merging #76 into master will increase coverage by 0.03%.
The diff coverage is 90.19%.

@@            Coverage Diff             @@
##           master      #76      +/-   ##
==========================================
+ Coverage    96.1%   96.13%   +0.03%     
==========================================
  Files          10       10              
  Lines         616      621       +5     
==========================================
+ Hits          592      597       +5     
  Misses         24       24

Impacted Files	Coverage Δ
src/blockmap.jl	98.59% <100%> (ø)	⬆️
src/wrappedmap.jl	100% <100%> (ø)	⬆️
src/linearcombination.jl	100% <100%> (+4.61%)	⬆️
src/transpose.jl	89.28% <100%> (+0.39%)	⬆️
src/LinearMaps.jl	83.07% <60%> (-2.38%)	⬇️
src/uniformscalingmap.jl	94.36% <93.33%> (-2.31%)	⬇️
src/kronecker.jl	100% <0%> (+1.94%)	⬆️

---

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 dd960a5...7cd8c98. Read the comment docs.

[dkarrasch]

@jagot Many thanks for your quick review.

I took some structure from another pending PR and merged it here. As I said in #75, we can get rid of the VERSION distinction by the trait. Also, this goes a step towards getting rid of the A_mul_B! stuff. When #74 is merged, one will simply define a 3-arg or 5-arg mul!, set the trait accordingly, and then handle transposes and adjoints via

mul!(y, ::TransposeMap{<:Any,<:SomeMap}, x [, alpha, beta])

or (for free) via invariance of the map type under adjoint/transpose

transpose(::SomeMap) = SomeMap(...)

[jagot]

With the new changes, I am even more happy!

[dkarrasch]

Now I have again the issue that locally tests pass, both when I run the runtests.jl script and when I test the package in pgk-mode. Again, everyone is kindly invited to check things out locally and report allocations. Otherwise, I'll turn off allocation testing, leaving a switch for local tests.

[jagot]

I ran the test suite, but I did not see any allocations reported. Where do I find those?

[dkarrasch]

They pop up on Travis. Good to have confirmation that they don't occur locally. On my other PR, I get failing zero-allocation tests when testing in pkg-mode, but not when I run the test script simply as a file.

[dkarrasch]

@Jutho Will you be able to find some time for a quick review the next days? This one should be relatively easy.

[Jutho]

Thanks for the reminder, I'll try to find some time tonight. Any other PRs which are ready for review? To what extent is introducing a mulstyle trait versus removing A_mul_B! and embracing 5-arg mul! two alternative solutions to the same problem?

[dkarrasch]

Great! I'd say these things are orthogonal. The mulstyle helps to elegantly check whether there exists an allocation-free 5-arg mul!. Eventually, some 5-arg mul! is available for all LinearMaps. Removing A_mul_B! is what I should have done back in the 0.7/1.0 transition, but didn't see how to do it at the time. So the other PR is "fixing" something that came up much earlier than 3- vs 5-arg multiplication.

I'll give a brief update on the other PRs in their respective threads.

[Jutho]

Approved, but I gave some suggestions, none truly essential, just in case you want more programming fun! :-)

[dkarrasch]

I'll give the allocation testing a try once more. I think I'll rename all multiplication helper functions to _mul!(as opposed to lincombmul!, kronmul! etc.) in the "overhaul" PRs.

[dkarrasch]

Seems like the allocation issue had something to do with my unclean handling of types vs singleton instances and whatnot. Once tests pass, I'll merge. I'd suggest to have #72 in and then (finally) release v2.6. Since removal of A_mul_B! and friends is kind of breaking (for users with own LinearMap subtypes, that should perhaps become a major release?