-
-
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
more generic broadcast over a single sparse matrix #19239
Merged
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.
The four method definitions above for handling common combinations of sparse matrices with
broadcast
scalars is not satisfying: A better solution would achieve the same for (1) allbroadcast
scalar types (Base.Broadcast.containertype(x) == Any
?) and (2) any combination (number, order, type mixture) ofbroadcast
scalars. One might achieve (2) via a generated function. But how one might simultaneously and gracefully achieve (1) eludes me. Thoughts much appreciated. 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.
I don't think you need a generated function. Maybe we can brainstorm with @pabloferz about this at some point soon? Maybe start a discourse thread? @pabloferz, are you on discourse.julialang.org yet?
In any case, it seems like that should be separate from this PR, which is only about the single-argument case.
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.
Al least for this case you might want to add another trait layer. E.g.
@stevengj I'm already on discourse, so feel free to ping me (
@pabloferz
) if you'd like to start a thread overe there.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.
The following snippet works for arbitrary combinations of broadcast scalars with a single sparse matrix, but the
(y...) = f(a, y...)
kills inference.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.
A possible solution would be to have (additionally to what you have above)
EDIT: There was a typo. This works with one
SparseMatrixCSC
and up to ten scalars but won't be inferable with two or moreSparseMatrixCSC
s.EDIT 2: Added a method, and it appears to be inferable with up to eight arguments (I'd say that should be enough).
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.
The hacky snippet below takes a different tack. In principle it should handle any combination of broadcast scalars and
SparseMatrixCSC
s; in practice type inference fails when (1) the number of arguments exceedsfoursix or (2) more than oneSparseMatrixCSC
is involved. (edited:)I'm sure a better approach exists, possibly e.g. inverting the call chain. Having a look at
@pabloferz
's suggestion now. Best!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.
Below are two evolutions of @pabloferz's suggestion. The first variation retains the helper type, which distills to a stack for leading scalar arguments. The second variation nixes the helper type and instead uses a tuple for the stack.
Both variations reduce the mutual recursion between
broadcast_c
andbroadcast_sp
to recursion simply inbroadcast_sp
, and punt on argument lists involving more than oneSparseMatrixCSC
.Manually extending this approach to handle argument lists involving two (or some larger small integer N)
SparseMatrixCSC
s seems straightforward,but incurs rapid code growth with N.How to extend this approach to an arbitrary number of(see followup post). But perhaps N = 2 or N = 3 would be good enough.SparseMatrixCSC
s isn't clear to meI will play with the N = 2 case of this approach next, and then attempt to improve (inference of) the more general approach I posted above.
Code common to both variations
Variation retaining helper type
Variation nixing helper type
Best!
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.
New versions capable (in principle) of handling any number of
SparseMatrixCSC
s follow below. None circumvent #19096. TL;DR The first version (A) is the best so far, perhaps good enough to incorporate for now? Thoughts? Any and all pointers much appreciated, particularly towards better understanding of why type inference works/fails where it does.Code common to all versions:
(A) For the following version, type inference remains happy when the argument list (1) contains a single sparse matrix and up to six broadcast scalars, (2) contains two sparse matrices and up to twelve broadcast scalars (more depending on position, strangely), or (3) up to six sparse matrices as long as no broadcast scalars appear. This behavior is a mystery to me, insight much appreciated. This version seems the best in practice at the moment.
Edit: Partial evaluation not correct.
(B) For the following version (a descendant of @pabloferz's suggestion), type inference remains happy when the argument list (1) contains up to nine members including either one or two sparse matrices, or (2) contains three sparse matrices and no broadcast scalars. This behavior is also a mystery to me, insight much appreciated.Edit: Partial evaluation not correct.
(C) The following version is a simpler realization of the preceding concept, but type inference does not seem to like it (nor a similar version using varags forgroupargs
).Best!
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.
@Sacha0 Inspired on your approach above that evolved from my suggestion I was able to write it in such a way that it is inferable up to eight arguments irrespectively of the arguments types. The maximum number of arguments, so that it keeps inferable, I believe, is a limitation of the inference mechanism, but I haven't looked this into detail.
The approach would be your 'common code' above plus
NOTE: When using the dot syntax, given that the parser can already reduce the effective number of arguments passed to broadcast the number of arguments can be even more.
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.
Discussion continued on discourse. Best!