Review sub devices #343
Review sub devices #343
Conversation
|
This supersedes #326 now to expedite merging of this work to master. |
@oleksandr-pavlyk @PokhodenkoSA @1e-to As-is this PR cannot be merged into master. It has work-in-progress commits with temporary commit messages that I will not merge into master. While we are still a very small beta library it can catch up soon. I want us to stick to something close to (if not as stringent as https://releases.llvm.org/8.0.1/docs/DeveloperPolicy.html#incremental-development). Thoughts? |
I am making the comment after trying to and giving up on cleaning up using interactive rebase, doing such a rebase is a huge drag on my time. |
1. Moved code for sub-devices to the end of class definition It was placed in the middle of initialization code block, separating `__cinit__` from `_init_*` routines it uses. 2. create_sub_devices requires use of keyword argument parition. Usage is d.create_sub_devices(partition=4) 3. Plugged possible memory leak in create_sub_devices_by_counts which memory allocated for array would not be freed if sub-devices vector was returned NULL 4. Changed signature of create_sub_device_by_counts from accepting object of type list, to accepting any object, which is checked to be Sized (len(obj) is expected to work), and Iterable, i.s. iteration of elements works 5. Changes to accept all integral types, such as np.int32, not just python int. 6. Got rid of _raise_create_sub_devices_exception function, and replaced it with explicit raise CreateSubDevicesError(message) 7. updated tests to use partition= keyword
oleksandr-pavlyk
force-pushed
the
review-sub-devices
branch
from
44ea8c0 to
8481251
Compare
|
Thanks @oleksandr-pavlyk for an excellent display of git-fu. |
We were able to squash all of @1e-to changes into two separate commits retaining her name as commit's author, one for C-API and one for Python part. My review changes were also squashed into a separate commit. This should not been needed. If git branch is used to keep lots of WIP/backup snapshots, and the intent is to squash changes, or reorganize them in any other way, this should be done by the author before PR is opened for a review. |
|
Good to go. Merging... |
|
@diptorupd You need to approve, please. |
|
Thanks @1e-to and @oleksandr-pavlyk |
Closes #290
Changes were added atop commits in #326, and this PR superseded #326.