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

DEP: deprecate ndarray.conjugate's no-op fall through for non-numeric types #9060

Merged
merged 1 commit into from
May 7, 2017

Conversation

longjon
Copy link
Contributor

@longjon longjon commented May 6, 2017

While np.conjugate errors on non-numeric types, ndarray.conjugate silently returns its argument, even though both are documented as being equivalent. This is due to a fall-through condition in ndarray.conjugate which is useful for real data (e.g., https://github.com/numpy/numpy/blob/master/numpy/lib/function_base.py#L3087) but not meaningful for non-numeric data.

This PR deprecates the fall-through for non-numeric dtypes, bringing ndarray.conjugate closer to np.conjugate by favoring its fail-fast behavior.

This was originally part of #9003.

@eric-wieser
Copy link
Member

eric-wieser commented May 6, 2017

Excellent, this exhibits the same UI bug

@@ -420,6 +420,18 @@ def test_int_dtypes(self):
self.assert_deprecated(op.div, args=(a,b))
dt2 = dt1

class TestNonNumericConjugate(_DeprecationTestCase):
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: should have another blank line before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed; was copying the style immediately above which is single-spaced!

Copy link
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.

Mirroring my approval from the other PR. Would be good to have a second opinion on the deprecation before merging, but otherwise I'd hope this can make 1.13

@longjon
Copy link
Contributor Author

longjon commented May 6, 2017

Sounds good! The deprecation and bugfix commits are actually independent now; would it be helpful if I split them into separate PRs?

@eric-wieser
Copy link
Member

Yeah, I guess if you did that I could at least pull the trigger on the bugfix without any further opinions

@longjon longjon changed the title BUG: ndarray.conjugate broken for custom dtypes (unlike np.conjugate): redux DEP: deprecate ndarray.conjugate's no-op fall through for non-numeric dtypes (unlike np.conjugate) May 6, 2017
@eric-wieser eric-wieser changed the title DEP: deprecate ndarray.conjugate's no-op fall through for non-numeric dtypes (unlike np.conjugate) DEP: deprecate ndarray.conjugate's no-op fall through for non-numeric types May 6, 2017
@eric-wieser eric-wieser removed this from the 1.13.0 release milestone May 6, 2017
@homu
Copy link
Contributor

homu commented May 6, 2017

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

@charris
Copy link
Member

charris commented May 7, 2017

I copied over the description from #9003.

@charris
Copy link
Member

charris commented May 7, 2017

Needs rebase on account of the release notes.

@charris charris added this to the 1.13.0 release milestone May 7, 2017
@charris
Copy link
Member

charris commented May 7, 2017

Yikes,

In [1]: a = ones(3)

In [2]: b = a.conjugate()

In [3]: b[...] = 0

In [4]: b
Out[4]: array([ 0.,  0.,  0.])

In [5]: a
Out[5]: array([ 0.,  0.,  0.])

That definitely looks like an unexpected result that needs a future warning and fix.

Although I see that this does not affect that behavior.

types

This behavior is contrary to np.conjugate (which will error on
non-numeric types), even though documentation claims they are identical.
This deprecation favors failing fast.
@eric-wieser
Copy link
Member

eric-wieser commented May 7, 2017

Although I see that this does not affect that behavior.

The original patch did, but this came with a hefty performance cost for np.cov.

It would be nice if np.conjugate(x, writable=False) were a thing, which allows short-circuiting of no-op operators, and could be used in np.cov. (#9069)

@longjon
Copy link
Contributor Author

longjon commented May 7, 2017

I've rebased and rewritten the description, since the old one didn't make a lot of sense here.

(Keeping in mind that this PR is about something else:)
@charris, right, this is the original behavior, it's currently relied upon as @eric-wieser notes, and I'm not sure that it's not the "right" behavior. E.g., transpose also returns a view, and one often wants to compute the Hermitian conjugate, .conj().T, which one would generally like to still be a view on real data. ndarray.real also returns a view on real data (though perhaps its property-ness is more suggestive of that).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants