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

extend sparse broadcast to one- and two-dimensional Arrays, better version #20009

Merged
merged 1 commit into from
Jan 25, 2017

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Jan 13, 2017

Like #20007, this pull request extends sparse broadcast[!] to one- and two-dimensional Arrays (via promotion to sparse, as with structured matrices), addressing #11474. Unlike #20007, this pull request confines (to the SparseArrays.HigherOrderFns module) changes that work around the issue described in #20007; it does not impact other consumers of Broadcast. Best!

@Sacha0 Sacha0 added sparse Sparse arrays broadcast Applying a function over a collection labels Jan 13, 2017
@Sacha0 Sacha0 added this to the 0.6.0 milestone Jan 13, 2017
@Sacha0
Copy link
Member Author

Sacha0 commented Jan 18, 2017

Absent objections or requests for time, I plan to merge this Thursday morning PST. Best!

@StefanKarpinski
Copy link
Member

No objections so far, looks like you can merge this, @Sacha0

Copy link
Contributor

@tkelman tkelman left a comment

Choose a reason for hiding this comment

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

one little question and a typo, otherwise lgtm

# combinations involving sparse arrays and PromoteToSparse collections continue in the promote-to-sparse funnel
promote_containertype(::Type{PromoteToSparse}, ::Type{AbstractSparseArray}) = PromoteToSparse
promote_containertype(::Type{AbstractSparseArray}, ::Type{PromoteToSparse}) = PromoteToSparse
# combinations involving Arrays and PromoteToSparse ecollections continue in the promote-to-sparse funnel
Copy link
Contributor

Choose a reason for hiding this comment

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

is "ecollections" a typo?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed on push. Thanks!

_spcontainertype{T<:AbstractArray}(::Type{T}) = AbstractArray
# need the following two methods to override the immediately preceding method
_spcontainertype{T<:StructuredMatrix}(::Type{T}) = PromoteToSparse
_spcontainertype{T<:SparseVecOrMat}(::Type{T}) = AbstractSparseArray
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be only concrete SparseVecOrMat, or cover the entire corresponding abstract types?

Copy link
Member Author

Choose a reason for hiding this comment

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

At present the outer (i.e. Broadcast's) container type promotion mechanism only funnels SparseVector and SparseMatrixCSC to sparse broadcast[!], so the inner (sparse broadcast[!]'s) container type promotion mechanism only need handle SparseVector and SparseMatrixCSC. Not capturing other <:AbstractSparseArray is intentional: sparse broadcast[!] isn't built to handle <:AbstractSparseArray that are not SparseVector or SparseMatrixCSC, there being no established interface for such <:AbstractSparseArrays.

If we want sparse broadcast[!] to handle combinations involving non-SparseVecOrMat <:AbstractSparseArray at this time, I can add a commit or open a separate PR promoting non-SparseVecOrMat <:AbstractSparseArray to SparseVector or SparseMatrixCSC as appropriate (as with structured matrices). Thoughts? Thanks!

Copy link
Contributor

@tkelman tkelman Jan 25, 2017

Choose a reason for hiding this comment

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

We should think through how the AbstractSparse types are used a bit, maybe not right here right now though. Anything that's assuming the exact CSC or concrete SparseVector data structures, fields, storage order etc should only be defined for those concrete types. But any other behavior we think should generically apply for all sparse arrays, I think it's worth widening some signatures and letting the definers of any concrete subtypes override more specific behaviors where they want.

I guess in this particular case the tradeoff would be between getting method errors vs dispatching to more generic dense behavior? The latter's likely better for now until we work out a better extensibility mechanism for how to get broadcast on custom array types to do interesting things.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess in this particular case the tradeoff would be between getting method errors vs dispatching to more generic dense behavior? The latter's likely better for now until we work out a better extensibility mechanism for how to get broadcast on custom array types to do interesting things.

Yes, and agreed. (Though for the promotion approach to work, definition of convert([SparseVector|SparseMatrixCSC], UnknownSparseVectorOrMatrixType) alone should suffice.) Best!

@Sacha0
Copy link
Member Author

Sacha0 commented Jan 25, 2017

No objections so far, looks like you can merge this, @Sacha0

Cheers, will merge once CI clears. (The latest change only touched a comment, but might as well.) Best!

@StefanKarpinski
Copy link
Member

Go for it, @Sacha0!

@Sacha0 Sacha0 merged commit a5ff787 into JuliaLang:master Jan 25, 2017
@Sacha0 Sacha0 deleted the okspbcarrays branch January 25, 2017 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
broadcast Applying a function over a collection sparse Sparse arrays
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants