BUG: ndarray.conjugate broken for custom dtypes (unlike np.conjugate) by longjon · Pull Request #9003 · numpy/numpy

longjon · 2017-04-27T01:07:58Z

In #4730, @andyjost noted that ndarray.conjugate was broken for arrays of objects. This was fixed in #4887 by @juliantaylor by extending the conditions guarding a no-op fall-through to exclude both complex- and object-typed arrays. Unfortunately this solution is not complete, because custom dtypes may be conjugateable without being objects. This patch simply removes the fall-through, which actually makes the behaviors of the np.conjugate ufunc and ndarray.conjugate the same as documented.

This will be slower if ndarray.conjugate is used (needlessly) on real data; but then, it's now the same speed as np.conjugate. (Is numpy exploiting the specific no-op behavior of ndarray.conjugate anywhere?)

This is especially relevant to @moble's https://github.com/moble/quaternion library, which will silently no-op when using ndarray.conjugate on non-zero-dim arrays.

moble · 2017-05-02T11:22:34Z

As explained here, I can verify that this patch does fix the problem in the quaternion library. Thanks!

eric-wieser · 2017-05-02T11:38:38Z

Note that this "breaks" the following:

np.array('hello world').conjugate()
np.array('2016', dtype='M').conjugate()
np.array((1, 2), dtype=[('a', int), ('b',int)]).conjugate()

It also means that x.conjugate() is x is now false for x = np.array(1), where previously this was true

eric-wieser · 2017-05-02T11:48:01Z

To make this not break (bad) old code, I think the test should become:

PyArray_ISNUMBER(self) || PyArray_ISOBJECT(self) || PyArray_ISUSERDEF(self)

and then the else branch should have a DEPRECATE call warning that in future ndarray.conjugate will be the same as np.conjugate and error on those inputs

moble · 2017-05-02T12:27:14Z

I can verify that using the original code with

    if (PyArray_ISCOMPLEX(self) || PyArray_ISOBJECT(self)) {

replaced by

    if (PyArray_ISNUMBER(self) || PyArray_ISOBJECT(self) || PyArray_ISUSERDEF(self)) {

also solves my problem.

longjon · 2017-05-02T20:30:24Z

Sounds good, I've updated the patch as suggested.

eric-wieser · 2017-05-02T20:38:55Z

Can you split this into a BUG commit and a DEP commit? It's nice to have deprecation commits stand alone.

Also needs:

a test in test_deprecations.py
a release note entry about the deprecation
a release note entry about int.conjugate() no longer returning a view

But otherwise looks good

longjon · 2017-05-03T01:16:22Z

Okay, I've done as you say. Let me know if you'd prefer separate TST and DOC commits instead of putting those changes in the DEP commit.

eric-wieser

All looks good to me. My only concern is whether no longer returning a view is an ok change - It'd be nice if another maintainer could weigh in.

Obviously there's a consistency benefit here, but there's also a performance cost.

eric-wieser · 2017-05-03T09:13:58Z

doc/release/1.13.0-notes.rst

Nitpick - belongs in previous commit, but I don't think that's worth your time to fix

homu · 2017-05-03T17:13:51Z

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

longjon · 2017-05-03T18:04:50Z

Rebased and moved release notes into correct commits.

Re: performance cost, I count five conjugations in this repo (outside tests) which are guarded by a complex check, and one which is not at https://github.com/numpy/numpy/blob/master/numpy/lib/function_base.py#L3087 in cov. That last one seems pretty important to me -- cov may be called on big arrays where making an extra copy would be bad news.

I could change this so that a view is still returned when PyArray_ISNUMBER && !PyArray_ISCOMPLEX, otherwise a new array is returned when PyArray_ISCOMPLEX || PyArray_ISOBJECT || PyArray_ISUSERDEF, otherwise a view is returned and we hit a deprecation warning. It'd still be a shame that np.conjugate behaves differently, but fixing that seems much deeper...

longjon · 2017-05-03T18:30:13Z

Since I find the cov issue sufficiently worrisome, I've gone ahead and implemented the above; now the first commit simply deprecates non-numeric ndarray.conjugate, and the second extends conjugation to types with PyArray_ISUSERDEF.

eric-wieser

Very thorough patch - thanks! Just some minor style nits, and this is good to merge

eric-wieser · 2017-05-03T23:02:21Z

numpy/core/src/multiarray/calculation.c

Convention would be to indent this twice, (doc\C_STYLE_GUIDE.rst.txt)

eric-wieser · 2017-05-03T23:02:25Z

numpy/core/src/multiarray/calculation.c

Indentation is weird here - should probably either be column aligned, or 8 spaces - currently, it's neither

longjon · 2017-05-03T23:39:42Z

Indentation updated.

eric-wieser · 2017-05-03T23:42:56Z

numpy/core/src/multiarray/calculation.c

One last thing - this needs a comment like NumPy 1.13, 2017-05-04 by the deprecation. Sorry for not spotting that in the last review

Comment added, following the format of other such comments.

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.

This is especially relevant to moble's numpy quaternion library, which will silently fail to conjugate when using ndarray.conjugate.

eric-wieser · 2017-05-04T14:05:16Z

Something screwy seems to be going on with github here, so I'm gonna try closing and reopening

(no test marked by the second commit, "view changes" button is a dead link. Also the "new changes" text won't go away)

eric-wieser · 2017-05-04T14:09:08Z

@charris: Any idea why travis is only testing the first commit (d425733) in this PR?

longjon · 2017-05-04T16:07:33Z

Travis is correctly testing the final commit (see, e.g., https://github.com/longjon/numpy/commits/d4257335fc515aff9cda64d57cd3ec72841b9720/numpy/core/src/multiarray/calculation.c). GitHub lists commits in chronological rather than topological order, because GitHub is designed around a concatenative rather than a rewriting workflow. (The other UI issues just sound broken though!)

homu · 2017-05-06T00:23:22Z

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

eric-wieser · 2017-05-06T00:30:16Z

In the interest of leaving this around as a testcase for the support thread I started with github about the UI, can I get you to make a new PR for rebasing this?

eric-wieser · 2017-05-06T00:43:42Z

Thanks @longjon

charris added 00 - Bug component: numpy._core labels Apr 27, 2017

jonlong-symbio mentioned this pull request May 1, 2017

Enable conjugation moble/quaternion#1

Closed

longjon force-pushed the always-conjugate branch 2 times, most recently from 8260ac9 to 8254690 Compare May 2, 2017 20:06

longjon force-pushed the always-conjugate branch 2 times, most recently from aa240fd to 76ab732 Compare May 3, 2017 00:23

eric-wieser approved these changes May 3, 2017

View reviewed changes

longjon force-pushed the always-conjugate branch from 76ab732 to 3898ef2 Compare May 3, 2017 17:38

longjon force-pushed the always-conjugate branch from 3898ef2 to cd90a1b Compare May 3, 2017 18:23

eric-wieser reviewed May 3, 2017

View reviewed changes

longjon force-pushed the always-conjugate branch from cd90a1b to 16a27f8 Compare May 3, 2017 23:21

eric-wieser reviewed May 3, 2017

View reviewed changes

longjon added 2 commits May 3, 2017 16:57

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

5fd3ce8

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.

BUG: ndarray.conjugate broken for custom dtypes (unlike np.conjugate)

d425733

This is especially relevant to moble's numpy quaternion library, which will silently fail to conjugate when using ndarray.conjugate.

longjon force-pushed the always-conjugate branch from 16a27f8 to d425733 Compare May 3, 2017 23:57

eric-wieser closed this May 4, 2017

eric-wieser reopened this May 4, 2017

longjon mentioned this pull request May 6, 2017

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

Merged

eric-wieser closed this May 6, 2017

longjon mentioned this pull request May 6, 2017

BUG: ndarray.conjugate broken for custom dtypes (unlike np.conjugate) #9061

Merged

eric-wieser mentioned this pull request May 12, 2017

BUG: Do not elide complex abs() #9110

Merged

Uh oh!

Conversation

longjon commented Apr 27, 2017

Uh oh!

moble commented May 2, 2017

Uh oh!

eric-wieser commented May 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eric-wieser commented May 2, 2017

Uh oh!

moble commented May 2, 2017

Uh oh!

longjon commented May 2, 2017

Uh oh!

eric-wieser commented May 2, 2017

Uh oh!

longjon commented May 3, 2017

Uh oh!

eric-wieser left a comment • 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!

eric-wieser May 3, 2017

Choose a reason for hiding this comment

Uh oh!

homu commented May 3, 2017

Uh oh!

longjon commented May 3, 2017

Uh oh!

longjon commented May 3, 2017

Uh oh!

eric-wieser left a comment

Choose a reason for hiding this comment

Uh oh!

eric-wieser May 3, 2017

Choose a reason for hiding this comment

Uh oh!

eric-wieser May 3, 2017

Choose a reason for hiding this comment

Uh oh!

longjon commented May 3, 2017

Uh oh!

eric-wieser May 3, 2017 • 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!

longjon May 4, 2017

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

longjon commented May 4, 2017

Uh oh!

homu commented May 6, 2017

Uh oh!

eric-wieser commented May 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eric-wieser commented May 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

eric-wieser commented May 2, 2017 •

edited

Loading

eric-wieser left a comment •

edited

Loading

eric-wieser May 3, 2017 •

edited

Loading

eric-wieser commented May 4, 2017 •

edited

Loading

eric-wieser commented May 4, 2017 •

edited

Loading

eric-wieser commented May 6, 2017 •

edited

Loading