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.
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
Slicing-based
np.block
implementation #306Slicing-based
np.block
implementation #306Changes from 2 commits
e6c5b02
2ebaf92
72e7500
55946df
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
"last outcome" => "outcome"
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 guess you could use
functools.chain
instead.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 think you had a chance to do this shape checking and input conversion in
check_list_depth
. Why are you doing this checking again here? Is it simply because you want to reusecheck_shape_dtype
to construct anArrayInfo
?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'd use
arrays
instead ofarr
as in the caller.arr
sounds like a single ndarray, although it's really a list of arrays.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.
Again,
common_info
seems to be optional, but you always take the shape of it. If it is never aNone
, then please turn it into a positional argument to document the behavior.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.
This is both a very bad naming (since
out_array
isn't an array!) and also untyped coding, as_concatenate
returns either a pair of a shape and a list or an ndarray. I think you should extract out the common code and separate the function forslicing_only=True
from that forslicing_only=False
.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.
slice(None)
would be sufficient.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.
do you think we should always use the slicing implementation? I know you were doing some experiments to figure out a reasonable threshold to toggle the behavior and if so, please leave a TODO comment.