API: Add .mT attribute for arrays by Kai-Striega · Pull Request #23762 · numpy/numpy

Kai-Striega · 2023-05-15T06:55:46Z

This is the initial commit for adding an mT, or matrix transpose, attribute for arrays. It is still a work in progress and not done yet. Please note that this is my first time working on NumPy's C-API. The PR is mainly being submitted this early to confirm that I am on the right track.

Things that are still needed:

Release note
Masked array support
~~[ ] np.transpose~~
Check input dimensions in array_matrix_transpose_get and exit early if 1d array
Tests fail for higher dimensional arrays
Figure out the correct location for the tests

Another note; ndarray.T has a corresponding function np.transpose, as part of this PR should we add a corresponding function for .mT e.g. np.matrix_transpose? If this is the case, should I add a function to PyArray_MatrixTranspose to shape.c rather than defining the function inside getset.c as is done with the regular transpose function?

numpy/core/src/multiarray/getset.c

numpy/core/tests/test_matrix_transpose.py

The lower dimensional (1d and 2d) tests compare the behaviour of `.mT` against `.T` not swapaxes.

The test generates random shapes of a given length. This was previously used with min_dims=3. This excludes arrays of dimension 0, 1, 2 which we want. Instead we test all dimensions up to 7.

seberg

Looks like a good start, will mark as draft for now if you don't mind, just unmark it when things are settling (not that I think there is much missing)!

numpy/core/src/multiarray/getset.c

numpy/core/tests/test_matrix_transpose.py

This commit reduces the memory required by the test. Instead of explicitly constructing an array of shape `shape`, we now construct only the last dim and broadcast over that.

Hypothesis seems to be overkill for the provided situation. Use explicitly defined values instead.

This commit causes `.mT` to raise a ValueError if ndim < 2. This is because the matrix transpose is undefined for 0 or 1 dimensional arrays and having it implemented may lead to confusion by users.

rgommers · 2023-05-17T10:12:35Z

Thanks for working on this Kai!

Another note; ndarray.T has a corresponding function np.transpose, as part of this PR should we add a corresponding function for .mT e.g. np.matrix_transpose?

I'd say yes we want that (because of the analogue to .T and because the array API standard has it). It doesn't have to be in this PR though, if it's already large and easier to do as a follow-up - whatever seems best to you.

charris · 2023-05-17T15:32:29Z

This will need a release note.

numpy/core/code_generators/numpy_api.py

numpy/core/src/multiarray/methods.c

Kai-Striega · 2023-05-21T04:17:04Z

@rgommers I'm happy to work on it in this PR, but I'm still learning about NumPy internals, meaning that the commit history could become quite messy as I learn (of course I'll try my best to keep it clean). As a summary, I think we should match the functions for transpose i.e.:

np.matrix_transpose
np.ndarray.mT
~~np.ndaray.matrix_transpose~~
np.ma.matrix_transpose
np.ma.MaskedArray.mT
~~np.ma.MaskedArray.matrix_transpose~~

The next step for me is to add np.ndaray.matrix_transpose, which I am having some trouble with. What I have done so far is:

Moved the implementation into a new function (PyArray_MatrixTranspose) into numpy/core/src/multiarray/shape.c alongside PyArray_Transpose
Added a matrix_transpose entry to array_methods in numpy/core/src/multiarray/methods.c
Added PyArray_Transpose to the multiarray_funcs_api in numpy/core/code_generators/numpy_api.py

I'm not really sure if this is the right direction, or if we even want to add PyArray_MatrixTranspose to the C-API as (I think) I am doing.

mattip · 2023-05-21T07:25:27Z

I would not touch the NumPy C-API with this PR. It is enough to add the attribute to ndarray and ma. If in the future we wish to export it via the C-API, that can be another PR.

rgommers · 2023-05-21T09:00:15Z

As a summary, I think we should match the functions for transpose i.e.:

We don't need/want both a .mT property and a .matrix_transpose method. It's duplicate and the plan is to trim down on ndarray methods. Just the property and the np.matrix_transpose function (and their MaskedArray equivalents) is enough.

charris · 2023-05-21T13:55:20Z

I rather like the idea of an .mT property :) It would be useful to have a short way to call it in ...@...@... constructions.

This was decided to not be needed in the PR review. Skipping CI as I know it doesn't work and don't need CI to tell me that. [skip ci]

Kai-Striega · 2023-05-22T00:47:46Z

I've removed the code adding it to the C-API (I think) and stopped working on adding an ndarray.matrix_transpose method. I've kept the function PyArray_MatrixTranspose in shape.c as I want to be able to share it between ndarray.mT and np.matrix_transpose - this is causing the test to segfault compared to the earlier version.

I may need some help structuring the code so that I can share it between ndarray.mT and np.matrix_transpose.

[skip ci]

seberg · 2023-05-22T17:32:50Z

You have to add the new function to a header file now and include it, probably. If you check the test failures, you see they fail on compiler warnings.

BvB93

Can you add ndarray.mT annotations to the main init.pyi stub file? The type is identical to ndarray.T so you can just reuse that it:

numpy/numpy/__init__.pyi

Lines 986 to 987 in cc0abd7

    
           @property 
        
           def T(self: _ArraySelf) -> _ArraySelf: ...

Kai-Striega · 2023-06-12T07:36:23Z

@seberg I've finally got the masked array implementation working! I'm not sure if it's done correctly, could you take a look at my last commit?

If it's right, I think I have three things left:

Take the reference to the function versions out of the release notes
Add the documentation
Add type hints for the masked array

seberg · 2023-06-12T07:43:54Z

numpy/ma/core.py

+
+        if self._mask is nomask:
+            # TODO: Should this return a masked_array or regular array?
+            return self._data.mT


A masked array. Otherwise, seems good. Although .transpose() has some _update_from_self stuff, it may make sense to just stick close to it in the off-chance it matters.

I'm struggling to replicate .transpose() as it uses the function version, not the attribute. I haven't been figured out how to do this (yet) and will leave it for another PR - maybe I can come back and tidy it up then if there are problems?

This function will be added in another PR.

Kai-Striega · 2023-06-12T13:08:08Z

@rgommers @seberg I've made the last changes that (I think) are needed, could I please ask you two for another review?

I've decided to leave the np.matrix_transpose function for now, I'll get to it in another PR

seberg

LGTM, thanks, just should add the versionadded.

numpy/core/_add_newdocs.py

numpy/ma/core.py

Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>

seberg · 2023-06-21T07:56:56Z

Thanks, lets put it in.

After merging, I noticed that the docs actually render slightly incorrectly. Not a big deal, but maybe you want to follow up?

I guess we need <BLANKLINE> or maybe its fine to just remove the blank line also.

Kai-Striega · 2023-06-22T00:13:30Z

Thanks that's my first C level contribution 👌. Let's hope for many more.

I'll follow up with the docs rendering properly.

ENH: Add .mT attribute for arrays

23f3651

github-actions bot added the 25 - WIP label May 15, 2023

Kai-Striega commented May 15, 2023

View reviewed changes

numpy/core/src/multiarray/getset.c Outdated Show resolved Hide resolved

numpy/core/tests/test_matrix_transpose.py Outdated Show resolved Hide resolved

numpy/core/tests/test_matrix_transpose.py Outdated Show resolved Hide resolved

Kai-Striega commented May 15, 2023

View reviewed changes

numpy/core/tests/test_matrix_transpose.py Outdated Show resolved Hide resolved

Kai-Striega commented May 15, 2023

View reviewed changes

numpy/core/tests/test_matrix_transpose.py Outdated Show resolved Hide resolved

Kai-Striega added 2 commits May 15, 2023 15:30

TST: Fix test names to reflect test behaviour

cdf850e

The lower dimensional (1d and 2d) tests compare the behaviour of `.mT` against `.T` not swapaxes.

TST: Increase max_dims for matrix_transpose test

13f0a2e

The test generates random shapes of a given length. This was previously used with min_dims=3. This excludes arrays of dimension 0, 1, 2 which we want. Instead we test all dimensions up to 7.

seberg reviewed May 15, 2023

View reviewed changes

numpy/core/src/multiarray/getset.c Outdated Show resolved Hide resolved

numpy/core/tests/test_matrix_transpose.py Outdated Show resolved Hide resolved

numpy/core/tests/test_matrix_transpose.py Outdated Show resolved Hide resolved

seberg marked this pull request as draft May 15, 2023 15:23

Kai-Striega added 4 commits May 17, 2023 12:10

TST: Use np.broadcast_to to save memory in test

086a919

This commit reduces the memory required by the test. Instead of explicitly constructing an array of shape `shape`, we now construct only the last dim and broadcast over that.

TST: Avoid using hypothesis in test case

95da479

Hypothesis seems to be overkill for the provided situation. Use explicitly defined values instead.

TST/MAINT: Rename file to be more generic

e33c002

ENH: Raise ValueError for ndim < 2 in .mT

5c346a3

This commit causes `.mT` to raise a ValueError if ndim < 2. This is because the matrix transpose is undefined for 0 or 1 dimensional arrays and having it implemented may lead to confusion by users.

Kai-Striega added 2 commits May 20, 2023 18:47

TST: Add test for the existence of matrix_transpose

58a0e0d

ENH: Try to create a matrix_transpose function

710aaed

Kai-Striega commented May 21, 2023

View reviewed changes

numpy/core/code_generators/numpy_api.py Outdated Show resolved Hide resolved

Kai-Striega commented May 21, 2023

View reviewed changes

numpy/core/src/multiarray/methods.c Outdated Show resolved Hide resolved

REV: Remove PyArray_MatrixTranspose from C-API

0304796

This was decided to not be needed in the PR review. Skipping CI as I know it doesn't work and don't need CI to tell me that. [skip ci]

DOC: Add release note for matrix transpose

bd40405

[skip ci]

BvB93 reviewed May 25, 2023

View reviewed changes

Kai-Striega added 2 commits May 26, 2023 15:37

TYP: Add typehint for new .mT attribute

8115919

ENH: Include PyArray_MatrixTranspose in shape.h

716e85c

Kai-Striega and others added 2 commits May 31, 2023 14:36

Merge branch 'numpy:main' into mT

6185d19

ENH/TST: First attempt to support MaskedArrays

f510534

seberg reviewed Jun 12, 2023

View reviewed changes

Kai-Striega added 5 commits June 12, 2023 16:38

ENH: Make ma.MaskedArray.mT return a masked array in all cases

6a9a938

TYP: ma: Add typehint for .mT

d256239

DOC: Document mT property for masked arrays

e244a27

REL: Remove matrix_transpose function from release note

f43dfcc

This function will be added in another PR.

DOC: Add documentation for ndarray.mT

7d3d371

Kai-Striega marked this pull request as ready for review June 12, 2023 13:03

InessaPawson added the triage review Issue/PR to be discussed at the next triage meeting label Jun 15, 2023

seberg approved these changes Jun 19, 2023

View reviewed changes

numpy/core/_add_newdocs.py Show resolved Hide resolved

numpy/ma/core.py Show resolved Hide resolved

DOC: Document version added

02529f1

Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>

seberg merged commit c6e1fd0 into numpy:main Jun 21, 2023

seberg changed the title ~~WIP: ENH: Add .mT attribute for arrays~~ API: Add .mT attribute for arrays Jun 21, 2023

Kai-Striega deleted the mT branch June 22, 2023 04:23

This was referenced Jun 24, 2023

ENH: Add np.matrix_transpose #24035

Closed

DOC: Fix documentation for ndarray.mT #24077

Merged

Uh oh!

Conversation

Kai-Striega commented May 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rgommers commented May 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

charris commented May 17, 2023

Uh oh!

Uh oh!

Uh oh!

Kai-Striega commented May 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattip commented May 21, 2023

Uh oh!

rgommers commented May 21, 2023

Uh oh!

charris commented May 21, 2023

Uh oh!

Kai-Striega commented May 22, 2023

Uh oh!

seberg commented May 22, 2023

Uh oh!

BvB93 left a comment

Choose a reason for hiding this comment

Uh oh!

Kai-Striega commented Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seberg Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

Kai-Striega Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

Kai-Striega commented Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

seberg commented Jun 21, 2023

Uh oh!

Kai-Striega commented Jun 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Kai-Striega commented May 15, 2023 •

edited

Loading

rgommers commented May 17, 2023 •

edited

Loading

Kai-Striega commented May 21, 2023 •

edited

Loading

Kai-Striega commented Jun 12, 2023 •

edited

Loading

Kai-Striega commented Jun 12, 2023 •

edited

Loading