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 .imag and .real properties to NamedArray #8365

Merged
merged 7 commits into from
Oct 24, 2023

Conversation

andersy005
Copy link
Member

@andersy005 andersy005 commented Oct 24, 2023

No description provided.

@github-actions github-actions bot added the topic-NamedArray Lightweight version of Variable label Oct 24, 2023
@andersy005 andersy005 requested a review from Illviljan October 24, 2023 03:03
@andersy005

This comment was marked as resolved.

@andersy005 andersy005 changed the title add .imag and .real properties and .astype method to NamedArray add .imag and .real properties to NamedArray Oct 24, 2023
@andersy005 andersy005 marked this pull request as ready for review October 24, 2023 10:27
@andersy005 andersy005 enabled auto-merge (squash) October 24, 2023 10:27
@andersy005 andersy005 merged commit ccc8f99 into pydata:main Oct 24, 2023
25 checks passed
@andersy005 andersy005 deleted the add-real-and-imag-to-namedarray branch October 24, 2023 16:50
@@ -516,6 +512,28 @@ def data(self, data: duckarray[Any, _DType_co]) -> None:
self._check_shape(data)
self._data = data

@property
def imag(self) -> Self:
Copy link
Contributor

Choose a reason for hiding this comment

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

When you use imag you're changing the dtype of the class, so it's not Self you're returning but rather a new NamedArray. Which won't match with self.

This should similar to the function, like:

def imag(
    self: NamedArray[_ShapeType, np.dtype[_SupportsImag[_ScalarType]]], /  # type: ignore[type-var]
) -> NamedArray[_ShapeType, np.dtype[_ScalarType]]:

Here's how numpy does it:
https://github.com/numpy/numpy/blob/14bb214bca49b167abc375fa873466a811e62102/numpy/__init__.pyi#L1495-L1500

Variable will also need this style of typing.

--------
numpy.ndarray.imag
"""
return self._replace(data=self.data.imag) # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Only use ._replace if the shape or dtype wont change.

return self._new(data=self.data.imag)

return self._replace(data=self.data.imag) # type: ignore

@property
def real(self) -> Self:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same idea here. Follow the same idea but use _SupportsReal instead.

--------
numpy.ndarray.real
"""
return self._replace(data=self.data.real) # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Same idea, dtype or shape has changed in this method so use ._new.

Comment on lines +170 to +177
def test_real_and_imag() -> None:
named_array: NamedArray[Any, Any]
named_array = NamedArray(["x"], np.arange(3) - 1j * np.arange(3))
expected_real = np.arange(3)
assert np.array_equal(named_array.real.data, expected_real)

expected_imag = -np.arange(3)
assert np.array_equal(named_array.imag.data, expected_imag)
Copy link
Contributor

Choose a reason for hiding this comment

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

You're not testing dtypes here. They don't match.

  • expected_imag is an int dtype.
  • named_array.imag.data is a float dtype.

The test should look something like this:

def test_real_and_imag() -> None:
    expected_real: np.ndarray[Any, np.dtype[np.float64]]
    expected_real = np.arange(3, dtype=np.float64)

    expected_imag: np.ndarray[Any, np.dtype[np.float64]]
    expected_imag = -np.arange(3, dtype=np.float64)

    arr: np.ndarray[Any, np.dtype[np.complex128]]
    arr = expected_real + 1j * expected_imag

    named_array: NamedArray[Any, np.dtype[np.complex128]]
    named_array = NamedArray(["x"], arr)

    actual_real: np.ndarray[Any, np.dtype[np.float64]] = named_array.real.data
    assert np.array_equal(actual_real, expected_real)
    assert actual_real.dtype == expected_real.dtype

    actual_imag: np.ndarray[Any, np.dtype[np.float64]] = named_array.imag.data
    assert np.array_equal(actual_imag, expected_imag)
    assert actual_imag.dtype == expected_imag.dtype

Copy link
Member Author

Choose a reason for hiding this comment

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

thank you for the review... i'm going to address these in a follow up PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-NamedArray Lightweight version of Variable
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants