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

ENH: implement __complex__ #7229

Merged
merged 3 commits into from
Mar 23, 2016
Merged

ENH: implement __complex__ #7229

merged 3 commits into from
Mar 23, 2016

Conversation

ewmoore
Copy link
Contributor

@ewmoore ewmoore commented Feb 11, 2016

Fixes gh-2491.

There is still some asymmetry here. Calling float() or complex() on a np.string_ scalar will try to convert the string to a float or a complex. But this still doesn't work for size = 1 arrays.

In [2]: float(np.array(['1.1']))
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-2-981e3b7ad7d1> in <module>()
----> 1 float(np.array(['1.1']))

TypeError: don't know how to convert scalar number to float

In [3]: float(np.array(['1.1'])[0])
Out[3]: 1.1

In [4]: complex(np.array(['1.1']))
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-4-a230266b5899> in <module>()
----> 1 complex(np.array(['1.1']))

TypeError: cannot convert to complex

In [5]: complex(np.array(['1.1'])[0])
Out[5]: (1.1+0j)

@seberg
Copy link
Member

seberg commented Feb 13, 2016

About time, someone does this. I guess it already works for scalars (or maybe they get forwarded to ndarray)?

Could you move this to the identical definitions for __float__, etc? Or is that not possible because __complex__ has no C-Api side definition?

@charris charris added 01 - Enhancement 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes component: numpy._core labels Feb 13, 2016
@charris
Copy link
Member

charris commented Feb 14, 2016

Needs release note.

@ewmoore
Copy link
Contributor Author

ewmoore commented Feb 15, 2016

It already works for scalars. I guess I could put it over with __float__, but since it doesn't have a C-Api definition I put it with the other methods like __copy__. I can move it if you want.

I've updated the PR to include a comment in the release notes and to handle object or string dtypes as well as to improved the error message provided when conversion cannot be done.

Supporting strings is mostly about completeness. Since it works for the scalar type it may as well work for the array type too. __float__ is broken in this case still.

@seberg
Copy link
Member

seberg commented Feb 15, 2016

Did not look at the code yet. Leaving it where it is sounds fine, since
the struct addition also is a point for here.
A comment next to the other ones is maybe a good idea, not sure.

No conversion from string please. Also I don't think it exists for the
scalars:

In [10]: a = np.array('1.j')
In [11]: a[()].__complex__()
----> 1 a[()].__complex__()

AttributeError: 'numpy.string_' object has no attribute '__complex__'

complex(a[()]) works, but that I guess that is because of coercion
from the string value that complex does, not because we say that it
is a reasonable thing to do.

@ewmoore
Copy link
Contributor Author

ewmoore commented Feb 15, 2016

I'll drop the string conversion then.

Mind that your example fails because complex() doesn't go via __complex__ for str types. Since np.str_ is a subclass of str it gets handled in the complex constructor via the string path. What this means in practice is exactly what I showed in the first comment, that scalars allow the conversion but arrays (even 0 dim arrays) do not.

@ewmoore
Copy link
Contributor Author

ewmoore commented Feb 15, 2016

Updated.

@homu
Copy link
Contributor

homu commented Feb 21, 2016

☔ The latest upstream changes (presumably #7296) made this pull request unmergeable. Please resolve the merge conflicts.

@ewmoore
Copy link
Contributor Author

ewmoore commented Feb 22, 2016

rebased.

@charris
Copy link
Member

charris commented Feb 22, 2016

Close and reopen to try to trigger appveyor.

@charris charris closed this Feb 22, 2016
@charris charris reopened this Feb 22, 2016
@seberg
Copy link
Member

seberg commented Mar 4, 2016

Hmmm, whats wrong with AppVoyer? I will try the close + reopen again. @ewmoore if you have some time, one thing that is missing are tests the failure cases as well, those would be good. Otherwise LGTM.

@seberg seberg closed this Mar 4, 2016
@seberg seberg reopened this Mar 4, 2016
@seberg
Copy link
Member

seberg commented Mar 4, 2016

Hmm, didn't help, but I very much doubt this has anything to do with the code, so....

}

if (!PyArray_CanCastArrayTo(self, dtype, NPY_SAME_KIND_CASTING) &&
!(PyArray_TYPE(self) == NPY_OBJECT)) {
Copy link
Member

Choose a reason for hiding this comment

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

Oh, would be nice if you can increase the indent here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@charris
Copy link
Member

charris commented Mar 4, 2016

The close, reopen trick doesn't work with appveyor.

@homu
Copy link
Contributor

homu commented Mar 5, 2016

☔ The latest upstream changes (presumably #7373) made this pull request unmergeable. Please resolve the merge conflicts.

@ewmoore
Copy link
Contributor Author

ewmoore commented Mar 22, 2016

Rebased, comments addressed.

@seberg
Copy link
Member

seberg commented Mar 22, 2016

Could you still add a test for the failure cases like more then 1 element in the array, and maybe some weird void type conversion to scalar?

@ewmoore
Copy link
Contributor Author

ewmoore commented Mar 23, 2016

Tests added.

@ewmoore
Copy link
Contributor Author

ewmoore commented Mar 23, 2016

I can squash this if you'd prefer.

@seberg
Copy link
Member

seberg commented Mar 23, 2016

Squashing is nice, but I can live with three reasonably described commits.

Thanks a lot Eric!

@seberg seberg merged commit 1380fdd into numpy:master Mar 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants