-
Notifications
You must be signed in to change notification settings - Fork 11
Bcast
object wrappers, attempt 2
#310
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
Conversation
3c4d373
to
62672f6
Compare
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.
Noticed one thing. Otherwise, I'm on board. 👍
One other thing that just occurred to me: do we need to add some unpacking for the case where the broadcastee is itself a container? (For example, adding a |
That's a fair question. I think I would prefer to wait for a specific use case and keep it simple until that point. One happy fact about this approach is that we can introduce additional |
62672f6
to
720e1ec
Compare
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.
Looks good to me too! Worth adding the simple non-recursive Bcast
version too?
720e1ec
to
0d4a555
Compare
I'm not in favor. I brought over the tests from #280, replaced all the |
Co-authored-by: Matt Smith <[email protected]>
0d4a555
to
fa488f1
Compare
Thanks all for your help with this! |
Second attempt at #280, much simpler technically.
I'm mostly seeking early feedback on this. It still needs, at least:
This could help fill the hole that's left by the plan to make implicit broadcasting of data class containers across actx array types illegal. See inducer/grudge#377 for an example use, specifically
shortcuts.py
. (Note that I've updated the grudge PR to use this.)