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 transpose function #939

Merged
merged 38 commits into from Aug 23, 2021
Merged

Add transpose function #939

merged 38 commits into from Aug 23, 2021

Conversation

EmilyBourne
Copy link
Member

@EmilyBourne EmilyBourne commented Aug 13, 2021

Implement the transpose function. In fortran when used as a temporary variable the transpose function is used. When the variable is saved, the ordering is used to ensure pointers are created. In C the function is handled through unravelling or with a new function transpose_alias_assign.

The transpose function is also used to implement the ndarray property .T and method .transpose()

Related to #806 and #458

@EmilyBourne EmilyBourne marked this pull request as ready for review August 16, 2021 14:12
@EmilyBourne
Copy link
Member Author

Fortran's transpose function only works for 2D arrays, so I expect that reshape should be used instead of transpose...

Fixed 018c0a1

@yguclu
Copy link
Member

yguclu commented Aug 23, 2021

@yguclu

Indeed I am puzzled by the fact that the 3D test works... We need to understand what is going on there!

It is strange. gfortran doesn't specify what happens if you pass something that is not 3d, so maybe they have implemented this to be helpful?

Possibly. Do you have access to an Intel or PGI compiler? At least one of them should raise an error...

@EmilyBourne EmilyBourne marked this pull request as draft August 23, 2021 15:05
@EmilyBourne
Copy link
Member Author

@EmilyBourne How do you handle the case of Numpy arrays with rank > 2? Fortran's transpose function only works for 2D arrays, so I expect that reshape should be used instead of transpose...

Hmm, it seems the tests are passing as there is no transpose required in most cases, it just changes the order of the resulting array. And in the case of an expression the transpose is often unrolled anyway. I have added a test case to force the use of the transpose function and it seems that the transpose is not correctly managed

@yguclu
Copy link
Member

yguclu commented Aug 23, 2021

Hmm, it seems the tests are passing as there is no transpose required in most cases, it just changes the order of the resulting array. And in the case of an expression the transpose is often unrolled anyway. I have added a test case to force the use of the transpose function and it seems that the transpose is not correctly managed

Thanks for looking into this, that makes a lot of sense! I am afraid that this PR will need a bit more work then, so it may be worth checking a couple of other things:

  • Do you handle the method a.transpose() in addition to the property a.T?
  • Can the user pass the *axes parameters to the function and method as described here?

@EmilyBourne
Copy link
Member Author

Do you handle the method a.transpose() in addition to the property a.T?

No, I will add this

Can the user pass the *axes parameters to the function and method?

No, I wanted to add this, but in python I believe it is possible to do this without necessarily allocating memory. This is not possible in Fortran unless we change all our element-wise access methods to carry axes instead of order (with order='C' equivalent to (0,1,2,3,...) and order='F' equivalent to (rank-1,rank-2,,...) ) so I thought it was probably complicated enough to leave for another PR.

@yguclu
Copy link
Member

yguclu commented Aug 23, 2021

Can the user pass the *axes parameters to the function and method?

No, I wanted to add this, but in python I believe it is possible to do this without necessarily allocating memory. This is not possible in Fortran unless we change all our element-wise access methods to carry axes instead of order (with order='C' equivalent to (0,1,2,3,...) and order='F' equivalent to (rank-1,rank-2,,...) ) so I thought it was probably complicated enough to leave for another PR.

I thought that even in Numpy a new array was created in all cases where it is not sufficient to change the ordering... I am going to check this.

@EmilyBourne
Copy link
Member Author

I thought that even in Numpy a new array was created in all cases where it is not sufficient to change the ordering... I am going to check this.

No, as long as it can be handled by modifying the steps array numpy (and c for us) does not need to create a new array. E.g:

> import numpy as np
> x = np.random.randint(50, size=(3,2,4))
> y=x.transpose((1,2,0))
> y.flags
  C_CONTIGUOUS : False
  F_CONTIGUOUS : False
  OWNDATA : False
  WRITEABLE : True
  ALIGNED : True
  WRITEBACKIFCOPY : False
  UPDATEIFCOPY : False

@yguclu
Copy link
Member

yguclu commented Aug 23, 2021

I thought that even in Numpy a new array was created in all cases where it is not sufficient to change the ordering... I am going to check this.

No, as long as it can be handled by modifying the steps array numpy (and c for us) does not need to create a new array. E.g:

> import numpy as np
> x = np.random.randint(50, size=(3,2,4))
> y=x.transpose((1,2,0))
> y.flags
  C_CONTIGUOUS : False
  F_CONTIGUOUS : False
  OWNDATA : False
  WRITEABLE : True
  ALIGNED : True
  WRITEBACKIFCOPY : False
  UPDATEIFCOPY : False

You're right. Upon calling transpose Numpy recomputes the strides and stores them in the array struct.

@EmilyBourne EmilyBourne removed the Ready_to_merge Approved by senior developer. Ready for final approval and merge label Aug 23, 2021
@yguclu
Copy link
Member

yguclu commented Aug 23, 2021

For now I suggest to add the transpose method, and raise a neat error if the user passes the *axes parameters.

@EmilyBourne
Copy link
Member Author

For now I suggest to add the transpose method, and raise a neat error if the user passes the *axes parameters.

Done. Should I open an issue for the *axes parameter?

@yguclu
Copy link
Member

yguclu commented Aug 23, 2021

@EmilyBourne I think the PR description should be expanded a bit (the method transpose and the property T should be mentioned). After that this can be merged.

@EmilyBourne EmilyBourne marked this pull request as ready for review August 23, 2021 16:25
@EmilyBourne EmilyBourne added the Ready_to_merge Approved by senior developer. Ready for final approval and merge label Aug 23, 2021
@yguclu yguclu merged commit d63d4f7 into master Aug 23, 2021
@yguclu yguclu deleted the ebourne_transpose branch August 23, 2021 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready_to_merge Approved by senior developer. Ready for final approval and merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants