Skip to content
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

Support for arrays in addition to tuples in LinearCombination #169

Closed
oschulz opened this issue Feb 16, 2022 · 8 comments · Fixed by #171
Closed

Support for arrays in addition to tuples in LinearCombination #169

oschulz opened this issue Feb 16, 2022 · 8 comments · Fixed by #171

Comments

@oschulz
Copy link
Contributor

oschulz commented Feb 16, 2022

We use LinearMaps extensively in MGVI.jl. We're adding support for Newton-CG-optimization now which needs means of linear maps. The number of maps in the mean/sum is not fixed though, so we'd like to use an array of maps instead of a tuple in LinearCombination. Would a pull request to extend LinearCombination that way be welcome?

@dkarrasch
Copy link
Member

I'm glad to hear LinearMaps.jl is used. @jagot took a similar approach in #84, but then didn't finish because it didn't do the job he needed, but you may find some inspiration there. On the other hand, mean (almost) already works.

using LinearMaps, Statistics
A = LinearMap(rand(3,3))
B = A + A + A
julia> mean(B.maps)
3×3 LinearMaps.ScaledMap{Float64} with scale: 0.3333333333333333 of
  3×3 LinearMaps.LinearCombination{Float64} with 3 maps:
    3×3 LinearMaps.WrappedMap{Float64} of
      3×3 Matrix{Float64}
    3×3 LinearMaps.ScaledMap{Float64} with scale: 1.0 of
      3×3 LinearMaps.WrappedMap{Float64} of
        3×3 Matrix{Float64}
    3×3 LinearMaps.WrappedMap{Float64} of
      3×3 Matrix{Float64}

The fact that the maps form a tuple shouldn't be a big restriction, because these tuples are all lazy, i.e., adding another map to an existing LinearCombination forms a new tuple (of pointers to the maps) of lenght one bigger than before. That should be really cheap and fast. If you have loads of maps, then tuples are no longer efficient, which was @jagot's motivation to generalize to vectors IIRC. Otherwise, we could add a method like

Statistics.mean(A::LinearCombination) = mean(A.maps)

which would add another dependency just for the sake of this one method, but that's not too bad. Oh, but wait, what would change if the maps field was a vector instead of a tuple? Another option is to just use the above definition in your code, i.e., if you know that you will have a B::LinearCombination at some point, then just hard-code mean(B.maps) instead of some generic mean(B), which wouldn't be generic after all because it doesn't make sense for any other map type. Let me know what you think.

@oschulz
Copy link
Contributor Author

oschulz commented Feb 16, 2022

because these tuples are all lazy, i.e., adding another map to an existing LinearCombination forms a new tuple (of pointers to the maps) of lenght one bigger than before

Yes, the problem is the load on the compiler, with new data types created all the time for tuples of different length (and n can be hundreds of even thousands in our use case). Mean itself works fine indeed already.

Statistics.mean(A::LinearCombination) = mean(A.maps)

Yes, forgot to mention, I would like to specialize mean and sum. And from what I can see LinearCombination should work just fine if we allow As<:Union{LinearMapTuple,AbstractArray{<:LinearMap} or similar.

Just wanted to check with you before preparing a PR.

@oschulz
Copy link
Contributor Author

oschulz commented Feb 16, 2022

I'm glad to hear LinearMaps.jl is used.

It's awesome! We found it to be a very powerful abstraction. We use it for lazy Jacobians too, with both left- and right-multiplication (backed by ForwardDiff and Zygote, by default).

@oschulz
Copy link
Contributor Author

oschulz commented Feb 16, 2022

CC MGVI devs @apmypb, @jknollm and @vindex10

@dkarrasch
Copy link
Member

Before you start with your own PR, check out #84. It does a few more things (to block concatenation), but it already has the LinearCombination extension that you want, including some adoption of the mul! machinery. So you could play with it and see how things work.

@oschulz
Copy link
Contributor Author

oschulz commented Feb 17, 2022

Thanks, will do! Should I just base a PR on top of #84?

@dkarrasch
Copy link
Member

That was just a suggestion for a quick start. It's not important which way you go. #84 has more things that we might keep for when there is actual interest in it.

@oschulz
Copy link
Contributor Author

oschulz commented Feb 17, 2022

Ok, I'll start with LinearCombination and specialized sum then.

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 a pull request may close this issue.

2 participants