Skip to content

ENH: Make np.complexfloating generic w.r.t. np.floating#17172

Merged
eric-wieser merged 2 commits intonumpy:masterfrom
BvB93:complexfloating
Sep 1, 2020
Merged

ENH: Make np.complexfloating generic w.r.t. np.floating#17172
eric-wieser merged 2 commits intonumpy:masterfrom
BvB93:complexfloating

Conversation

@BvB93
Copy link
Copy Markdown
Member

@BvB93 BvB93 commented Aug 27, 2020 •
edited
Loading

This pull requests makes np.complexfloating generic with respect to np.floating.

The main advantage of this is to make it easier to access the .real/.imag type of
np.complexfloating sub-classes as is illustrated below.

Examples

from typing import TypeVar
import numpy as np

FloatType = TypeVar('FloatType', bound=np.floating)

def get_real(complex_value: 'np.compexfloating[FloatType]') -> FloatType:
    return complex_value.real

@BvB93 BvB93 changed the title ENH: Make np.complexfloating generic w.r.t. to np.floating ENH: Make np.complexfloating generic w.r.t. np.floating Aug 27, 2020
eric-wieser
eric-wieser reviewed Aug 27, 2020
View reviewed changes
numpy/__init__.pyi
@property
def real(self) -> _FloatType: ...
@property
def imag(self) -> _FloatType: ...
Copy link
Copy Markdown
Member

@eric-wieser eric-wieser Aug 27, 2020

Choose a reason for hiding this comment

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

Aren't these properties of np.number anyway?

Copy link
Copy Markdown
Member Author

@BvB93 BvB93 Aug 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

They are and they were already defined for np.number some time ago

Hang on, for some reason this _real_generic mixing is subclassed in a number of np.number sub-classes but not np.number itself. This doesn't seem right.

numpy/numpy/__init__.pyi

Lines 381 to 387 in 32b3f82

class _real_generic(generic): # type: ignore
@property
def real(self: _ArraySelf) -> _ArraySelf: ...
@property
def imag(self: _ArraySelf) -> _ArraySelf: ...
class number(generic): ... # type: ignore

What makes np.complexfloating somewhat unique is that the return type of
.real and .imag is not type(self); it's one of the np.floating subclasses.

Copy link
Copy Markdown
Member

@eric-wieser eric-wieser Aug 27, 2020

Choose a reason for hiding this comment

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

I think we forgot that integers support real and imag too, to match python

Copy link
Copy Markdown
Member Author

@BvB93 BvB93 Aug 27, 2020

Choose a reason for hiding this comment

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

I think we forgot that integers support real and imag too, to match python

I think we're safe here actually, as _real_generic is included in the definition of integer:

numpy/numpy/__init__.pyi

Line 407 in 32b3f82

class integer(number, _real_generic): ... # type: ignore

Still, it would in my opinion be cleaner to just move real and imag to the np.generic superclass.
As both properties have to be redefined for np.complexfloating anyway (due to their distinct return type)
the original concern should not be an issue (numpy/numpy-stubs#14 (comment)).

Copy link
Copy Markdown
Member

@eric-wieser eric-wieser Aug 27, 2020

Choose a reason for hiding this comment

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

I think number makes more sense than generic, as the result of str_.real is nonsense

Copy link
Copy Markdown
Member Author

@BvB93 BvB93 Aug 28, 2020

Choose a reason for hiding this comment

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

That might for the better, yes.
I had to double check but real and imag will apparently be deprecated for non-numeric types anyway (#10818).

In any case, I'm planning to save this for a future maintenance pull request as there are also a few other issues I'd like to fix in one go (e.g. str and bytes are missing from a couple of __init__()s).

@eric-wieser
Copy link
Copy Markdown
Member

eric-wieser commented Aug 27, 2020 •
edited
Loading

I don't understand, should that just be

from typing import TypeVar
import numpy as np

FloatType = TypeVar('FloatType', bound=np.inexact)

def get_real(value: FloatType) -> FloatType:
    return value.real

@eric-wieser
Copy link
Copy Markdown
Member

eric-wieser commented Aug 27, 2020

Ah nevermind, I understand now

eric-wieser
eric-wieser reviewed Aug 28, 2020
View reviewed changes
numpy/__init__.pyi
def real(self) -> _FloatType: ...
@property
def imag(self) -> _FloatType: ...
def __abs__(self) -> _FloatType: ... # type: ignore[override]
Copy link
Copy Markdown
Member

@eric-wieser eric-wieser Aug 28, 2020

Choose a reason for hiding this comment

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

Why is ignore needed here but not on the others?

Copy link
Copy Markdown
Member Author

@BvB93 BvB93 Aug 28, 2020

Choose a reason for hiding this comment

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

__abs__() is already defined a super-class while the other two are not (as per our previous real/imag discussion):

numpy/numpy/__init__.pyi

Line 256 in 32b3f82

def __abs__(self: _ArraySelf) -> _ArraySelf: ...

Mypy will complain due to incompatibilities between the super- and sub-types' signatures,
complaints which are silenced by # type: ignore[override].

For more details: https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides

eric-wieser
eric-wieser approved these changes Aug 28, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

Looks fine, we can clean up number.real and number.imag later.

@BvB93 BvB93 mentioned this pull request Sep 1, 2020
Closed
@eric-wieser eric-wieser merged commit 3fbc84a into numpy:master Sep 1, 2020
@BvB93 BvB93 deleted the complexfloating branch September 1, 2020 12:12
@BvB93 BvB93 mentioned this pull request Sep 1, 2020
Merged
@charris charris mentioned this pull request Oct 10, 2020
Closed
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eric-wieser eric-wieser eric-wieser approved these changes

Assignees

No one assigned

Labels

01 - Enhancement 41 - Static typing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@BvB93 @eric-wieser