ENH: Add namedtuple return types to linalg functions that return tuples by asmeurer · Pull Request #22786 · numpy/numpy · GitHub

That is, eig(), eigh(), qr(), slogdet(), and svd(). For those functions that return non-tuples with certain keyword arguments, the return type is unchanged. This change should be completely backwards compatible.

The namedtuple attribute names come from the array API specification (see, e.g.,
https://data-apis.org/array-api/latest/extensions/generated/signatures.linalg.eigh.html), with the exception of eig() which is just the same as eigh(). The name of the namedtuple object itself is not part of the specification or the public API.

I have not used a namedtuple for the tuple output for qr(mode='raw'), which returns (h, tau).

This updates the docstrings to use the updated namedtuple return names, and also the examples to use those names more consistently. This also updates the tests to check each function for the namedtuple attributes at least once.

That is, eig(), eigh(), qr(), slogdet(), and svd(). For those functions that return non-tuples with certain keyword arguments, the return type is unchanged. This change should be completely backwards compatible. The namedtuple attribute names come from the array API specification (see, e.g., https://data-apis.org/array-api/latest/extensions/generated/signatures.linalg.eigh.html), with the exception of eig() which is just the same as eigh(). The name of the namedtuple object itself is not part of the specification or the public API. I have not used a namedtuple for the tuple output for qr(mode='raw'), which returns (h, tau). This updates the docstrings to use the updated namedtuple return names, and also the examples to use those names more consistently. This also updates the tests to check each function for the namedtuple attributes at least once.

Curious about this, it isn't the same as collections.namedtuple. Is typing.NamedTuple the compatible version?

Bit curious about the choice of upper case here and below. Is there a standard for these names?

This NDArray type name is already present in NumPy. Unless you mean the name of the namedtuple.

Public names of type annotations we have, like np.typing.NDArray, tend to be upper-case. I think to not confuse them with lower-case objects

I was referring to "Q".

Unless you mean the name of the namedtuple.

That was what I was referring to. Looks like NamedTuple is what we want, but am curious if the projects that we want to be compatible with also use it.

The uppercase field names are part of the array API spec and were discussed here data-apis/array-api#295. I suppose NumPy could, if it wanted, provide the same fields under multiple names if you really don't like the ones the array API chose, like

class QRResult(NamedTuple):
    Q: NDArray
    R: NDArray

    @property 
    def q(self):
        return self.Q

    @property 
    def r(self):
        return self.R

(although personally I think you should just use these names)

The name of the named tuple is completely arbitrary. We can change it to whatever.

As far as I can tell NamedTuple is basically equivalent to the classic namedtuple. I think the biggest difference is that NamedTuple supports type annotations, which aren't used in this file yet but presumably they will be at some point. I guess it also makes it easier to add other methods or attributes if you ever wanted to do that.

I don't think it really makes a difference whether you use NamedTuple or namedtuple. I think it could even be something else like a dataclass, so long as it also has the correct field names and also defines enough methods to act like a tuple.

As far as I can tell NamedTuple is basically equivalent to the classic namedtuple

I think NamedTuple is better, for instance, it allows accessing the fields like so: a.Q, but that wouldn't work for namedtuple. The declaration of the fields also differs, for instance the declarations here error if namedtuple is substituted. The upshot is that I think one or the other should be chosen for the standard for portable code.

namedtuple works like that too:

>>> namedtuple('test', ['a', 'b'])(1, 2).a
1

I think the only real difference is that NamedTuple lets you specify type annotations. And the class definition is arguably easier to read. As far as the user is concerned though they are identical.

Could you fix the lint errors? They don't look very difficult.

I would lean towards also using a named tuple for (h, tau), it seems strange to not be consistent for a single function. But not a strong opinion.

The changes need to be copied into the typing stubs as well still. Otherwise, I expect the only thing would indeed be small style fixups to make the linter happier.

The changes need to be copied into the typing stubs as well still

@asmeurer Be good to finish this up.

We should finish this before the 1.25 release.

The name of the namedtuple object itself is not part of the specification or the public API.

What are users expected to do when writing a type annotation for the return value, if ret : QRResult = qr(x) is using private API?

So maybe the namedtuple classes should be added to the API then. Maybe @BvB93 can give some insight? I'm not really knowledgeable enough about typing stuff to know the right answer here.

I think there's still issues with NamedTuple typing in Python (e.g., this is still true probably: https://stackoverflow.com/questions/60430110/python-typing-support-for-namedtuple). I suspect that the linalg.pyi stubs for these functions need a tweak here, and that tuple unpacking will still work unchanged with mypy. Actually typing the use of qr(x).Q seems tricky.

So maybe the namedtuple classes should be added to the API then.

Not to the np.linalg API, because they're not useful for anything but static typing, and that's not enough of a reason to add them to the public (runtime) API I'd say. We do have numpy.typing for type annotations that users may need, so they could be added there perhaps? That right now has only four entries, so this would grow it quite a bit - but that seems fine. Perhaps would need to be np.typing.linalg.xxx?

I suppose it would also be reasonable to think about the static types later; at worst users of heavy static typing will have to continue to use the tuple api rather than .Q and .R, which is no different to what they do today.

I think there's still issues with NamedTuple typing in Python (e.g., this is still true probably: https://stackoverflow.com/questions/60430110/python-typing-support-for-namedtuple). I suspect that the linalg.pyi stubs for these functions need a tweak here, and that tuple unpacking will still work unchanged with mypy. Actually typing the use of qr(x).Q seems tricky.

Runtime support for generic (i.e. parameterizable) named tuples was (re?)-introduced last year again (xref python/cpython#92027), so the issue mentioned in stackoverflow here should be irrelevant now. That, and the named tuples introduced in this PR are currently not generic anyway.

So maybe the namedtuple classes should be added to the API then. Maybe @BvB93 can give some insight? I'm not really knowledgeable enough about typing stuff to know the right answer here.

Named tuples do use a nominal sub-typing pattern just like normal classes (and contrary to protocols), so would have to expose these named tuples somewhere lest, as pointed out by Eric, the users only option would be to resort to the use of private numpy API.

That right now has only four entries, so this would grow it quite a bit - but that seems fine. Perhaps would need to be np.typing.linalg.xxx?

Either that, or if the returning of named tuples is going to be a common thing then something akin to np.typing.named_tuple maybe?

Either that, or if the returning of named tuples is going to be a common thing then something akin to np.typing.named_tuple maybe?

We don't have that many functions that return multiple arrays which can be named sensibly, so I suspect this isn't necessary.

Branching for 1.25 will be approaching, might be nice to push this over the finish line.

Co-authored-by: Bas van Beek <43369155+BvB93@users.noreply.github.com>

The circleci failure seems to be because the main version of .circleci/config.yml is not being used. Will ignore.

Thanks @asmeurer and @BvB93 .

Just linking because I didn't really expect this pattern of someone noticing: HIPS/autograd#597 (comment)

They have a dictionary for the result types, which probably only contains tuple.

🤔 I think I'd consider that an issue in their code, changing tuple to namedtuple seems like a thing that is okay to do, just like changing any other return type to a more specific subclass.

asmeurer added 3 commits December 12, 2022 18:23

Add a changelog entry for numpy#22786

6618fbf

Fix a doctest

271876e

charris reviewed Dec 13, 2022

View reviewed changes

seberg added this to the 1.25.0 release milestone Apr 4, 2023

BvB93 reviewed Apr 5, 2023

View reviewed changes

Comment thread numpy/linalg/linalg.py Outdated

Comment thread numpy/linalg/linalg.py

charris and others added 2 commits May 17, 2023 12:22

MAINT: Update numpy/linalg/linalg.py

610f85c

Co-authored-by: Bas van Beek <43369155+BvB93@users.noreply.github.com>

TYP: Update type annotations for the new linalg named tuples

a4c2496

charris merged commit 126b46c into numpy:main May 17, 2023

               import functools
               import operator
               import warnings
+              from typing import NamedTuple

ENH: Add namedtuple return types to linalg functions that return tuples by asmeurer · Pull Request #22786 · numpy/numpy · GitHub

Repository navigation

ENH: Add namedtuple return types to linalg functions that return tuples - #22786

ENH: Add namedtuple return types to linalg functions that return tuples#22786charris merged 5 commits intonumpy:mainnumpy/numpy:mainfrom asmeurer:linalg-namedtuplesasmeurer/numpy:linalg-namedtuplesCopy head branch name to clipboard

Conversation

asmeurer commented Dec 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asmeurer Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charris commented Dec 13, 2022

Uh oh!

seberg commented Dec 20, 2022

Uh oh!

charris commented Feb 23, 2023

Uh oh!

charris commented Apr 4, 2023

Uh oh!

eric-wieser commented Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asmeurer commented Apr 4, 2023

Uh oh!

rgommers commented Apr 5, 2023

Uh oh!

eric-wieser commented Apr 5, 2023

Uh oh!

BvB93 commented Apr 5, 2023

Uh oh!

Uh oh!

Uh oh!

rgommers commented Apr 6, 2023

Uh oh!

seberg commented May 8, 2023

Uh oh!

charris commented May 17, 2023

Uh oh!

charris commented May 17, 2023

Uh oh!

seberg commented Jun 21, 2023

Uh oh!

rgommers commented Jun 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ENH: Add namedtuple return types to linalg functions that return tuples#22786
charris merged 5 commits into
numpy:mainfrom
asmeurer:linalg-namedtuples

asmeurer Dec 15, 2022 •
edited
Loading

eric-wieser commented Apr 4, 2023 •
edited
Loading