8000 MAINT: Temporarily disable __numpy_ufunc__ for 1.10 by njsmith · Pull Request #6102 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

MAINT: Temporarily disable __numpy_ufunc__ for 1.10 #6102

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

MAINT: Temporarily disable __numpy_ufunc__ for 1.10 #6102

wants to merge 1 commit into from

Conversation

njsmith
Copy link
Member
@njsmith njsmith commented Jul 22, 2015

Following discussion in gh-5844, we regretfully decided that we have to
disable __numpy_ufunc__ again for 1.10. This patch should be reverted
on master after 1.10 is branched.

@njsmith njsmith mentioned this pull request Jul 22, 2015
Closed
8000
@matthew-brett
Copy link
Contributor

Is this really the right decision given the discussion that has gone before?

For example, if you only offered these two outcomes for a vote:

A ) disable __numpy_ufunc__ for 1.10;
B) option 1 in the previous discussion for 1.10;

what would the consensus be? From a superficial reading of this thread, it seems that most of y'all would prefer B). Is that not the case?

@njsmith
Copy link
Member Author
njsmith commented Jul 22, 2015

My understanding of how this maps to consensus voting is: I'm vetoing
options 2 and 3 on the grounds that I don't think we're ready to commit to
any particular variant of them, Pauli has a preference for option 3 but not
strong enough to veto anything himself, and Chuck is vetoing option 1
because of his own preferences plus he's not comfortable with violating
Pauli's preference. So that leaves "option 0". Which is all fair enough
IMHO even if it isn't anyone's most preferred outcome. Getting 1.10
unblocked is also pretty urgent; there's a ton of stuff in there that has
nothing to do with this.
On Jul 22, 2015 5:03 AM, "Matthew Brett" notifications@github.com wrote:

Is this really the right decision given the discussion that has gone
before?

For example, if you only offered these two outcomes for a vote:

A ) disable numpy_ufunc for 1.10;
B) option 1 in the previous discussion for 1.10;

what would the consensus be? From a superficial reading of this thread, it
seems that most of y'all would prefer B). Is that not the case?

—
Reply to this email directly or view it on GitHub
#6102 (comment).

@matthew-brett
Copy link
Contributor

Yes, I took Chuck to be following Pauli on this. I interpreted Pauli to not be against option 1, but thinking we'd need something on top of that. So then I'm assuming Pauli >= +0 on option 1. Is that right Pauli? If that's the case - Chuck - are you using a veto here?

@charris
Copy link
Member
charris commented Jul 22, 2015

@matthew-brett Yes. I think we've made some progress and should be able to get things settled given time. There is also the index removal and possible rename that should be in place before inclusion.

@charris
Copy link
Member
charris commented Jul 22, 2015

The dot override needs a fix.


---

Traceback (most recent call last):

  File "/home/travis/build/numpy/numpy/builds/venv/lib/python2.6/site-packages/numpy/core/tests/test_multiarray.py", line 4025, in test_dot_override

assert_equal(np.dot(a, b), "A")


TypeError: unsupported operand type(s) for *: 'A' and 'B'```

@juliantaylor
Copy link
Contributor

I am a bit worried that if we haven't managed to get a consensus after a year, is another year going to even help?
as I understand it we would have to rename it and keep the current numpy_ufunc around forever anyway as its already used.
I think the maintenance overhead of a third variant should we make one for 1.11 might be acceptable.

@charris
Copy link
Member
charris commented Jul 22, 2015

@juliantaylor it hasn't been a year of consensus seeking. The whole thing was off the radar until two/three months ago when the 1.10 release began to rev up. IIRC, a similar thing happened with the 1.9. I think if it is a planned part of 1.11 things will go better.

@matthew-brett
Copy link
Contributor

It seems to me there is, after a lot of hard patient work, substantial agreement that option 1 is better than removing __numpy_ufunc__. Chuck - I guess you agree that is true? In that case it seems to me there should be a compellling reason not to choose option 1, and I don't at the moment see what that is.

@charris
Copy link
Member
charris commented Jul 22, 2015

@matthew-brett It is time to move on.

@shoyer
Copy link
Member
shoyer commented Jul 22, 2015

I am a bit worried that if we haven't managed to get a consensus after a year, is another year going to even help?

This was also my worry, but I think we have some reasons to hope. We made some progress in that exhaustive discussion but there are still some loose ends. There are quite a few different use cases and design approaches to support.

I'm also optimistic that 1.11 is something that we'll be able to release in 4 months, not a year. Let's just not let this fall off the radar again.

@juliantaylor
Copy link
Contributor

we also said that about 1.10 and 1.9, and probably also 1.8, it never works out that way. Alone getting 1.10 from alpha to release will probably take 3 month.

@njsmith njsmith force-pushed the disable-numpy-ufunc-for-1.10 branch from 67c793f to 17dcc79 Compare July 23, 2015 01:55
@njsmith
Copy link
Member Author
njsmith commented Jul 23, 2015

The dot override needs a fix.

Not sure how I missed that... fixed.

@charris charris added this to the 1.10.0 release milestone Jul 24, 2015
charris
charris reviewed Jul 24, 2015
View reviewed changes
numpy/core/src/private/ufunc_override.h
* Temporarily disable this functionality for the 1.10 release.
* See gh-5844.
****************************************************************/
*result = NULL;
Copy link
Member

Choose a reason for hiding this comment

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

There are declarations after this code, leading to errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@njsmith
MAINT: Temporarily disable __numpy_ufunc__ for 1.10
e2dd12a 
Following discussion in numpygh-5844, we regretfully decided that we have to
disable __numpy_ufunc__ again for 1.10. This patch should be reverted
on master after 1.10 is branched.
@njsmith njsmith force-pushed the disable-numpy-ufunc-for-1.10 branch from 17dcc79 to e2dd12a Compare July 24, 2015 06:48
@charris
Copy link
Member
charris commented Jul 25, 2015

Nose is broken for Python 3.6-dev, we should probably disable the nightly test until it is fixed.

ERROR: test suite for <module 'test_format' from '/home/travis/build/numpy/numpy/builds/venv/lib/python3.6/site-packages/numpy/lib/tests/test_format.py'>

----------------------------------------------------------------------

Traceback (most recent call last):

  File "/home/travis/build/numpy/numpy/builds/venv/lib/python3.6/site-packages/nose/suite.py", line 210, in run

    self.setUp()

  File "/home/travis/build/numpy/numpy/builds/venv/lib/python3.6/site-packages/nose/suite.py", line 293, in setUp

    self.setupContext(ancestor)

  File "/home/travis/build/numpy/numpy/builds/venv/lib/python3.6/site-packages/nose/suite.py", line 316, in setupContext

    try_run(context, names)

  File "/home/travis/build/numpy/numpy/builds/venv/lib/python3.6/site-packages/nose/util.py", line 453, in try_run

    inspect.getargspec(func)

AttributeError: module 'inspect' has no attribute 'getargspec'

@charris
Copy link
Member
charris commented Jul 25, 2015

Or at least use the 3.5 branch instead.

@charris
Copy link
Member
charris commented Jul 25, 2015

Should be fixed by #6113

@charris charris closed this Jul 25, 2015
@charris charris reopened this Jul 25, 2015
@charris charris closed this Jul 25, 2015
@charris charris reopened this Jul 25, 2015
@charris
Copy link
Member
charris commented Jul 25, 2015

Close and reopen to reinitialize the travis tests.

@homu
Copy link
Contributor
homu commented Jul 25, 2015

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

@charris
Copy link
Member
charris commented Jul 26, 2015

@njsmith I'll take care of the merge conflicts.

@njsmith
Copy link
Member Author
njsmith commented Jul 26, 2015

Oh, gotcha, thanks!
On Jul 25, 2015 5:17 PM, "Charles Harris" notifications@github.com wrote:

@njsmith https://github.com/njsmith I'll take care of the merge
conflicts.

—
Reply to this email directly or view it on GitHub
#6102 (comment).

@charris charris mentioned this pull request Aug 3, 2015
Merged
@charris
Copy link
Member
charris commented Aug 3, 2015

Backported to 1.10 branch, so closing this.

@charris charris closed this Aug 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
03 - Maintenance component: numpy._core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@njsmith @matthew-brett @charris @juliantaylor @shoyer @homu @rgommers
0