-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
+107
−35
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 funnelsSparseVector
andSparseMatrixCSC
to sparsebroadcast[!]
, so the inner (sparsebroadcast[!]
's) container type promotion mechanism only need handleSparseVector
andSparseMatrixCSC
. Not capturing other<:AbstractSparseArray
is intentional: sparsebroadcast[!]
isn't built to handle<:AbstractSparseArray
that are notSparseVector
orSparseMatrixCSC
, there being no established interface for such<:AbstractSparseArray
s.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
toSparseVector
orSparseMatrixCSC
as appropriate (as with structured matrices). Thoughts? Thanks!There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and agreed. (Though for the promotion approach to work, definition of
convert([SparseVector|SparseMatrixCSC], UnknownSparseVectorOrMatrixType)
alone should suffice.) Best!