Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

API: Add .mT attribute for arrays#23762

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
seberg merged 21 commits intonumpy:mainfromKai-Striega:mT
Jun 21, 2023
Merged

API: Add .mT attribute for arrays#23762

seberg merged 21 commits intonumpy:mainfromKai-Striega:mT
Jun 21, 2023

Conversation

@Kai-Striega
Copy link
Member

@Kai-StriegaKai-Striega commentedMay 15, 2023
edited
Loading

This is the initial commit for adding anmT, 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 inarray_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 functionnp.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 toPyArray_MatrixTranspose toshape.c rather than defining the function insidegetset.c as is done with the regular transpose function?

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 usedwith min_dims=3. This excludes arrays of dimension 0, 1, 2 which we want.Instead we test all dimensions up to 7.
Copy link
Member

@sebergseberg left a comment

Choose a reason for hiding this comment

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

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)!

@sebergseberg marked this pull request as draftMay 15, 2023 15:23
This commit reduces the memory required by the test. Instead of explicitlyconstructing an array of shape `shape`, we now construct only the last dimand broadcast over that.
Hypothesis seems to be overkill for the provided situation. Use explicitlydefined values instead.
This commit causes `.mT` to raise a ValueError if ndim < 2. This is becausethe matrix transpose is undefined for 0 or 1 dimensional arrays and havingit implemented may lead to confusion by users.
@rgommers
Copy link
Member

rgommers commentedMay 17, 2023
edited
Loading

Thanks for working on this Kai!

Another note;ndarray.T has a corresponding functionnp.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 becausethe 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
Copy link
Member

This will need a release note.

seberg reacted with thumbs up emoji

@Kai-Striega
Copy link
MemberAuthor

Kai-Striega commentedMay 21, 2023
edited
Loading

@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 fortranspose 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 addnp.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) intonumpy/core/src/multiarray/shape.c alongsidePyArray_Transpose
  • Added amatrix_transpose entry toarray_methods innumpy/core/src/multiarray/methods.c
  • AddedPyArray_Transpose to themultiarray_funcs_api innumpy/core/code_generators/numpy_api.py

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

@mattip
Copy link
Member

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.

Kai-Striega and rgommers reacted with thumbs up emoji

@rgommers
Copy link
Member

As a summary, I think we should match the functions fortranspose 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 onndarray methods. Just the property and thenp.matrix_transpose function (and their MaskedArray equivalents) is enough.

@charris
Copy link
Member

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 needCI to tell me that.[skip ci]
@Kai-Striega
Copy link
MemberAuthor

I've removed the code adding it to the C-API (I think) and stopped working on adding anndarray.matrix_transpose method. I've kept the functionPyArray_MatrixTranspose inshape.c as I want to be able to share it betweenndarray.mT andnp.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 betweenndarray.mT andnp.matrix_transpose.

@seberg
Copy link
Member

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.

Copy link
Member

@BvB93BvB93 left a comment

Choose a reason for hiding this comment

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

Can you addndarray.mT annotations to the maininit.pyi stub file? The type is identical tondarray.T so you can just reuse that it:

@property
defT(self:_ArraySelf)->_ArraySelf: ...

Kai-Striega reacted with thumbs up emoji
@Kai-Striega
Copy link
MemberAuthor

Kai-Striega commentedJun 12, 2023
edited
Loading

@seberg I'vefinally 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


ifself._maskisnomask:
# TODO: Should this return a masked_array or regular array?
returnself._data.mT
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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?

@Kai-StriegaKai-Striega marked this pull request as ready for reviewJune 12, 2023 13:03
@Kai-Striega
Copy link
MemberAuthor

Kai-Striega commentedJun 12, 2023
edited
Loading

@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 thenp.matrix_transpose function for now, I'll get to it in another PR

@InessaPawsonInessaPawson added the triage reviewIssue/PR to be discussed at the next triage meeting labelJun 15, 2023
Copy link
Member

@sebergseberg left a comment

Choose a reason for hiding this comment

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

LGTM, thanks, just should add the versionadded.

Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>
@sebergseberg merged commitc6e1fd0 intonumpy:mainJun 21, 2023
@seberg
Copy link
Member

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.

InessaPawson reacted with hooray emoji

@sebergseberg changed the titleWIP: ENH: Add .mT attribute for arraysAPI: Add .mT attribute for arraysJun 21, 2023
@Kai-Striega
Copy link
MemberAuthor

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

I'll follow up with the docs rendering properly.

melissawm reacted with hooray emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@BvB93BvB93BvB93 left review comments

@sebergsebergseberg approved these changes

Assignees

No one assigned

Labels

25 - WIPtriage reviewIssue/PR to be discussed at the next triage meeting

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@Kai-Striega@rgommers@charris@mattip@seberg@BvB93@InessaPawson

[8]ページ先頭

©2009-2025 Movatter.jp