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.
This isn't correct. This should instead be based on contiguity. Column-major arrays for example allow concatenation along the last axis.
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 noticed that the code failed the tests because I missed a syntax error. Sorry for the noise.
About contiguity, I did some tests, and the function does not work in any axis for colum-major arrays. In line 2571 of the Array.concatenate function, the output array
result
is always a row-major array, leading to the sameNotImlementedError
due to miss-matching strides.Simple example:
It shouldn't be hard to allow column-major arrays, it is just a matter of initializing the result array with
order="F"
in these cases. The issue I see is that the Array class has no member that indicates the ordering. One could deduce the order by looking at the strides (if Array.strides[0] > Array.strides[-1]: row-major, else: column-major) but maybe it is too hacky. what do you think?Another approach is the one taken in the Array.stack function, in which when the axis != 0, it is assumed that the input is column mayor. But it would also lead to unclear error messages.
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.
Thanks for pointing out the issue with
concatenate
and column-major arrays, we should look into fixing that. I agree with your assessment about inspection of the strides, it's not quite elegant.FWIW, CI is still not passing on this branch.
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.
Oh yeah! I forgot to push. It should work now.