-
Notifications
You must be signed in to change notification settings - Fork 1
Block redistribution #584
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
Block redistribution #584
Conversation
Conflicts: distarray/globalapi/context.py
Necessary with redistribution, since we're calling functions on processes where an array might not be defined.
Conflicts: distarray/globalapi/context.py
distarray/globalapi/context.py
Outdated
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.
Needs a docstring addition for default.
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.
And actually, what is default for? A default value when something is a Proxy but doesn't have a dereference attribute? Or is it when .dereference() raises an AttributeError over .name or something?
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 refactored things so default is no longer necessary, and added a long explanatory comment.
|
General notes:
|
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.
Tests of a couple of error conditions would be nice. I assume you get a "Distributions not compatible" error if the shapes don't match up?
|
Seems like Shape too smallShape too largeUnsupported dist typeHere |
|
This is great! Summary of issues, as I see them:
|
Refactor `arg_kwarg_proxy_converter` into function in metadata_utils, add explanatory comment.
Also allow `distribute_as` to take `shape_or_dist`.
|
@bgrant all comments addressed, except for the |
|
Sounds good. We should consider supporting a single negative dimension size as well, like numpy's reshape does. |
Working implementation of block (and no-dist) redistribution.
Two main cases:
Both cases support redistribution on a different set of targets. The source and destination targets can be identical, overlapping, or disjoint.
Some small performance optimizations are implemented, but there is much room for improvement. This version just gets general redistribution working; future work will need to address performance and optimizations.