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 creating Applied/Broadcasted mixes via @~ #31

Merged
merged 2 commits into from
Apr 30, 2019

Conversation

tkf
Copy link
Member

@tkf tkf commented Apr 29, 2019

This PR implements my suggestion #21 (comment) so that we can do

Y .= @~ α .* (A * x) .+ β .* c

The main changes are:

  1. @~ converts normal calls to applied.
  2. An abstract type LazyArray is added as suggested in Lazy broadcasting macro #21 (comment). It is the supertype of BroadcastArray and ApplyArray.
  3. @~ does not wrap Broadcasted in BroadcastArray anymore. Instead, users have to call LazyArray explicitly.
  4. @~ calls instantiate on Broadcasteds (this is the same as Faster mapreduce for Broadcasted JuliaLang/julia#31020)

@mcabbott probably does not like that we need to write sum(LazyArray(@~ ...)) to get the speedup. However, that's only until JuliaLang/julia#31020 is merged.

I think it's nice to have an API that does not uses inference API so that y .= @~ ... can avoid inference overhead. It may be useful, e.g., inside a tight loop.

@codecov-io
Copy link

codecov-io commented Apr 29, 2019

Codecov Report

Merging #31 into master will increase coverage by 1.25%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #31      +/-   ##
==========================================
+ Coverage   55.19%   56.44%   +1.25%     
==========================================
  Files          14       15       +1     
  Lines         924      946      +22     
==========================================
+ Hits          510      534      +24     
+ Misses        414      412       -2
Impacted Files Coverage Δ
src/LazyArrays.jl 100% <ø> (ø) ⬆️
src/lazybroadcasting.jl 31.42% <100%> (-6.87%) ⬇️
src/lazyapplying.jl 70.58% <100%> (+0.58%) ⬆️
src/lazymacro.jl 78.78% <78.78%> (ø)
src/linalg/mul.jl 50% <0%> (+2.56%) ⬆️
src/linalg/lazymul.jl 12.12% <0%> (+3.03%) ⬆️
src/linalg/add.jl 55% <0%> (+5%) ⬆️

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 1c63bd0...37e8474. Read the comment docs.

@dlfivefifty
Copy link
Member

What do you think about using Applied instead of hacking Broadcasted for the BLAS MulAdd operations? That is,

Y .= @~ α .* (A * x) .+ β .* c

would become

Y .= @~ α * A * x + β * c

which is code for

Y .= applied(+, applied(*, α, A, x), applied(*, β, c))

@dlfivefifty
Copy link
Member

Using ApplyStyle we could also have a much more robust way of rearranging complicated MulAdd operations to reduce to MulAdd.

@tkf
Copy link
Member Author

tkf commented Apr 29, 2019

@~ α * A * x + β * c sounds good to me. It looks cleaner.

In a long run, I think it would be nice if both @~ α .* (A * x) .+ β .* c and @~ α * A * x + β * c can be "normalized" to a same representation via instantiate mechanism. This way, users don't have to know when to use, e.g., + or .+ to invoke performance specializations.

@dlfivefifty
Copy link
Member

Yes, I suppose both could work by an intelligent use of ApplyStyle and BroadcastStyle.

In the meantime, I'll go ahead and merge this PR.

@dlfivefifty dlfivefifty merged commit 1f111a9 into JuliaArrays:master Apr 30, 2019
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