Skip to content
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

Add support of np.swapaxes #4074 #6883

Merged
merged 4 commits into from Apr 21, 2021
Merged

Add support of np.swapaxes #4074 #6883

merged 4 commits into from Apr 21, 2021

Conversation

braniii
Copy link
Contributor

@braniii braniii commented Apr 1, 2021

This PR is adds np.swapaxes, which is currently unimplemented according to #4074 (or np.swapaxis which is the mis-spelling version). I had help by @stuartarchibald on discourse: (https://numba.discourse.group/t/how-to-use-types-integer-for-indexing-list/647)

This function can be used to simplify the implementation of np.rot90 from #6822, which is also the first step of implementing full further arguments of it.

@esc
Copy link
Member

esc commented Apr 1, 2021

@braniii thank you for submitting this, I have added it to the queue for review.

@stuartarchibald stuartarchibald added the Effort - medium Medium size effort needed label Apr 9, 2021
@stuartarchibald
Copy link
Contributor

This PR is adds np.swapaxes, which is currently unimplemented according to #4074 (or np.swapaxis which is the name in numpy <1.16).

I can't find reference to np.swapaxis anywhere in the git log or change log apart from in places where there's people correcting the spelling to swapaxes, has it ever existed or is it just a common typo/mis-spelling?

Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this feature. I've given this an initial review, there's a few small things to resolve but the patch is generally good. Thanks again!

numba/np/arrayobj.py Show resolved Hide resolved
numba/np/arrayobj.py Show resolved Hide resolved

axes_tuple = tuple_setitem(axes_list, axis1, axis2)
axes_tuple = tuple_setitem(axes_tuple, axis2, axis1)
return np.transpose(arr, axes_tuple)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm reasonably convinced that this will break the "swapaxes returns a view" notion as per the docs, https://numpy.org/doc/stable/reference/generated/numpy.swapaxes.html:

For NumPy >= 1.10.0, if a is an ndarray, then a view of a is returned; otherwise a new array is created.

Asserting that a is expected and a is got in the unittests might highlight this.

Copy link
Contributor Author

@braniii braniii Apr 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a is expected and a is got should always be false (except when exchanging with the same axis). For being true, not only the data but also the view needs to be the same (which gets changed) or one needs to use the base or np.share_memory.

You are right, that the definition breaks the "swapaxes returns a view". But the reason is, that the numba version of np.transpose does not return a view. Here a short code snippet:

import numpy as np
from numba import njit


@njita: False
def transpose(a):
    return np.transpose(a)


# reference arraype command to continue
a = np.arange(8).reshape(2, -1)

b = np.transpose(a)
c = transpose(a)

# this is always false
print(f'b is a: {b is a}')  # false
print(f'c is a: {c is a}')  # false

# using base it works
print(f'b.base is a.base: {b.base is a.base}')  # true
print(f'c.base is a.base: {c.base is a.base}')  # false

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's much we can do about this right now? Do you feel that this behaviour is prohibitive to it being useful as-is?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least from my side there is not much I can do, I have not enough inside in the code base to propose a fix. But I think the function is useful as-is. But in general it could be helpful to document difference from numpy functions, maybe simple at
https://numba.readthedocs.io/en/stable/reference/numpysupported.html

- `np.transpose()`
+ `np.transpose()` (a copy is returned)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least from my side there is not much I can do, I have not enough inside in the code base to propose a fix. But I think the function is useful as-is. But in general it could be helpful to document difference from numpy functions, maybe simple at
https://numba.readthedocs.io/en/stable/reference/numpysupported.html

- `np.transpose()`
+ `np.transpose()` (a copy is returned)

Agree, this is probably the best thing to do for now.

@stuartarchibald stuartarchibald added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Apr 9, 2021
@braniii
Copy link
Contributor Author

braniii commented Apr 9, 2021

This PR is adds np.swapaxes, which is currently unimplemented according to #4074 (or np.swapaxis which is the name in numpy <1.16).

I can't find reference to np.swapaxis anywhere in the git log or change log apart from in places where there's people correcting the spelling to swapaxes, has it ever existed or is it just a common typo/mis-spelling?

When I searched for the function I found multiple results, and I did not consider that numba could be wrong ;) But you are right, I corrected my comment. And thx for the review.

@stuartarchibald stuartarchibald added this to the Numba 0.54 RC milestone Apr 16, 2021
Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch and fixes!

@stuartarchibald stuartarchibald added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on author Waiting for author to respond to review labels Apr 21, 2021
@sklam sklam merged commit 5b34afb into numba:master Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to merge Review and testing done, is ready to merge Effort - medium Medium size effort needed enhancement numpy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants