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

Implement broadcast_axis #133

Merged
merged 4 commits into from
Mar 17, 2021
Merged

Implement broadcast_axis #133

merged 4 commits into from
Mar 17, 2021

Conversation

Tokazama
Copy link
Member

@Tokazama Tokazama commented Mar 8, 2021

This is essentially a formalization of _bcs1 that is called within the combine_axes function for broadcasting.

I'm sure we want to do some more stuff for broadcasting later, but for now I'm just trying to get some basic stuff in for combining axes. I'm not sure if we want the function that iteratively calls broadcast_axis to be combine_axes. We may want something more like combine_layout so that it can can account for reconstructing various types of sparsely populated arrays, which would involve more than just axes.

@codecov
Copy link

codecov bot commented Mar 8, 2021

Codecov Report

Merging #133 (94f7e3e) into master (614f34d) will increase coverage by 0.22%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #133      +/-   ##
==========================================
+ Coverage   85.68%   85.91%   +0.22%     
==========================================
  Files           9       10       +1     
  Lines        1411     1434      +23     
==========================================
+ Hits         1209     1232      +23     
  Misses        202      202              
Impacted Files Coverage Δ
src/ArrayInterface.jl 85.18% <ø> (ø)
src/broadcast.jl 100.00% <100.00%> (ø)

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 614f34d...94f7e3e. Read the comment docs.

@Tokazama
Copy link
Member Author

Tokazama commented Mar 9, 2021

Does this look good for now? It lacks the context of the entire broadcasting system but I figured it would be more manageable to approach it in a series of small PRs.

Also, combine_axes would probably be a better name but that's already used for combine_axes(A::AbstractArray...) upstream of this method. Open to any ideas.

@Tokazama Tokazama requested a review from chriselrod March 9, 2021 02:21
@chriselrod
Copy link
Collaborator

chriselrod commented Mar 9, 2021

There are a few re-implementations of broadcasting within the package ecosystem. Two include:

  1. LoopVectorization (and StrideArrays sort of does its own)
  2. @.. from DiffEq

I think we should look to these and what they need to decide what can be done to help them.

Aside from these packages (any others?), what are the goals/objectives here?

@ChrisRackauckas
Copy link
Member

@.. adds ivdep which breaks standard broadcast assumptions.

@Tokazama
Copy link
Member Author

Tokazama commented Mar 9, 2021

Aside from these packages (any others?), what are the goals/objectives here?

Currently Julia puts most of the responsibility of handling axes on the array provider. But arrays can provide different types of axes and they interact in one of the following ways

  1. Concatenate
  2. Merge
  3. Broadcast/combine

'indices' and 'eachindex' are specific instances for combining them and aren't necessarily suited for reconstructing axes in place for a new array.

I actually was looking at those packages when doing these and I haven't found anything that does exactly this. I think it would make sense to change the upstream function from combine_axes to combine_layouts, but I don't have enough experience with things like block arrays to know if that really is helpful. I'm not aware of a commonly used method that combines two arrays to produce something other than a dense array.

@Tokazama
Copy link
Member Author

Tokazama commented Mar 9, 2021

I should probably give some more context for this.

Whether the broadcasting struct (something like Broadcasted) is passed to instantiate or materialize the axes have to be combined in some way. Typically these are passed to similar, allowing each element produced by the broadcasted function to be computed in place.
The most obvious benefit of having something like broadcast_axis is it helps properties related to the axis, resolving issues likes this JuliaArrays/StaticArrays.jl#744.

It also helps with #130, where we might not actually be broadcasting something bet we are performing some sort of operation on two arrays that needs a destination that isn't a fully materialized array.

@Tokazama
Copy link
Member Author

Was the consensus that I needed to address how we're going to interact with the entirety broadcasting before moving forward with this?

@chriselrod
Copy link
Collaborator

chriselrod commented Mar 16, 2021

Sorry for being slow here. What is the interface/way for someone to use this?

Is it something someone opts into by defining new range types (returned by axes), broadcast styles, or that a different broadcasting implementation opts into?

@Tokazama
Copy link
Member Author

Tokazama commented Mar 16, 2021

The big pictures is that I'm trying to get a coherent interface for breaking down arrays into their memory and structural layout so we can have generic methods that help us reconstruct arrays (like similar but more flexible).

I've packaged the code I've been working on around related to this here. This line is where broadcasting stuff starts to diverge from Base.Broadcast when combine_layouts is called instead of combined_axes. I didn't include any of the layout stuff in this PR because it would require figuring out buffers too and I was trying to keep this one simple. I can add it in here if it makes more since though.

Is it something someone opts into by defining new range types (returned by axes), broadcast styles, or that a different broadcasting implementation opts into?

We could just us a different subtype of AbstractBroadcasted and have it diverge at instantiate and/or materialize!.

Copy link
Collaborator

@chriselrod chriselrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, sounds good.

@Tokazama Tokazama merged commit 903e6ef into master Mar 17, 2021
@Tokazama Tokazama deleted the broadcast_axis branch April 5, 2021 02:43
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