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.

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

BUG: Fix partition and argpartition error for empty input. Closes #6530 #6553

Merged
merged 3 commits into from
Oct 27, 2015
Merged

BUG: Fix partition and argpartition error for empty input. Closes #6530 #6553

merged 3 commits into from
Oct 27, 2015

Conversation

yashmehrotra
Copy link
Contributor

Fixes #6530 .

@yashmehrotra
Copy link
Contributor Author

@njsmith @jaimefrio It is throwing a deprecation warning in one test case. What should I do ? I had 2 things in mind, either use return 0 where the error is raised (in here ) or add a check in test cases to skip the test for that particular environment.
What are your opinions ? Is there any better way ?

@yashmehrotra
Copy link
Contributor Author

@njsmith @jaimefrio Nevermind , its resolved. The problem was that the 32 bit system was trying to cast array into 64 bit. I used dtype=int16 to avoid that.

@charris charris added this to the 1.10.2 release milestone Oct 24, 2015
@charris charris changed the title Fixed partition and argpartition error for empty input. Closes #6530 BUG: Fix partition and argpartition error for empty input. Closes #6530 Oct 24, 2015
@charris
Copy link
Member

charris commented Oct 25, 2015

@jaimefrio This look good to you? The fix is as you suggested.

@jaimefrio
Copy link
Member

There are goto fails, at least in lines 832, 972, 981 and 988 that would need a ret = -1 before them. Not sure if I am missing any other failure path that was relying on the initialization of ret...

@charris
Copy link
Member

charris commented Oct 26, 2015

@jaimefrio Can you find time to put together a PR? @yashmehrotra No offence intended, but I want this fix soon for 1.10.2 and would prefer if someone familiar with the code got it done rather than go back and forth with corrections.

@yashmehrotra
Copy link
Contributor Author

@charris It's ok, I understand, but I'd still commit the new changes that @jaimefrio mentioned. If it is working, we can merge it else I'll close this PR 😄 .

njsmith
njsmith reviewed Oct 27, 2015
View reviewed changes
numpy/core/tests/test_item_selection.py
@@ -68,6 +68,21 @@ def test_unicode_mode(self):
k = b'\xc3\xa4'.decode("UTF8")
assert_raises(ValueError, d.take, 5, mode=k)

def test_partition(self):
Copy link
Member

Choose a reason for hiding this comment

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

should be called test_empty_partition to make clear what's special about this test

@njsmith
Copy link
Member

njsmith commented Oct 27, 2015

@charris: I did some careful review of the actual change to item_selection.c, and I'm fairly confident that it's correct. (The actual problem is simpler than I realized.) The tests need some tweaks as per above, but they're relatively trivial. Otherwise LGTM.

njsmith
njsmith reviewed Oct 27, 2015
View reviewed changes
numpy/core/tests/test_item_selection.py

assert_equal(a, None)

def test_argpartition(self):
Copy link
Member

Choose a reason for hiding this comment

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

oh right, and this should be called test_empty_argpartition

@njsmith
Copy link
Member

njsmith commented Oct 27, 2015

OK, one more comment about tests and then I promise I'm done :-). We should also the following to numpy/core/tests/test_regression.py:

    def test_empty_percentile(self):
        # gh-6530 / gh-6553
        assert_array_equal(np.percentile(np.arange(10), []), np.array([]))

because this was the call that I actually ran into the problem with (percentile calls partition internally). So it should be fixed by this, but we should add a test to make sure it stays fixed too.

@yashmehrotra
Copy link
Contributor Author

@njsmith Oh, cool. I added the regression test. I hope its good to go now.

@njsmith
Copy link
Member

njsmith commented Oct 27, 2015

LGTM, but I'll let @charris decide as release manager how he wants to handle this (presumably after confirming that the tests actually pass ;-)).

@yashmehrotra
Copy link
Contributor Author

@njsmith Sure, No problem.

charris added a commit that referenced this pull request Oct 27, 2015
@charris
Merge pull request #6553 from yashmehrotra/partition-fix
c0e48cf 
BUG: Fix partition and argpartition error for empty input. Closes #6530
@charris charris merged commit c0e48cf into numpy:master Oct 27, 2015
@charris
Copy link
Member

charris commented Oct 27, 2015

@yashmehrotra Thanks, merged.

charris pushed a commit to charris/numpy that referenced this pull request Oct 27, 2015
@yashmehrotra @charris
TST: Added regression test empty percentile, in ref to numpy#6530 and n…
ac772b9 
…umpy#6553
@charris charris removed this from the 1.10.2 release milestone Oct 27, 2015
@jaimefrio
Copy link
Member

Thanks for cleaning up my mess, all three of you!

@charris
Copy link
Member

charris commented Oct 28, 2015

If you buy us beer and plane tickets we'll help you move ;)

@yashmehrotra
Copy link
Contributor Author

+1. I am with charles on this one. 😛

@njsmith
Copy link
Member

njsmith commented Oct 28, 2015

No no, you call it a "dev meeting" and get some company to pay for it ;-)

On Tue, Oct 27, 2015 at 9:17 PM, Charles Harris notifications@github.com
wrote:

If you buy us beer and plane tickets we'll help you move ;)

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

Nathaniel J. Smith -- http://vorpus.org

tpn added a commit to pyparallel/numpy that referenced this pull request Oct 30, 2015
@tpn
Merge branch 'master' of https://github.com/numpy/numpy
68772a2 
* 'master' of https://github.com/numpy/numpy: (384 commits)
  BUG: fix MANIFEST.in for removal of a file in numpygh-8047.
  DOC: Release notes for Numpy 1.10.2.
  MAINT: remove useless files with outdated info from repo root and doc/.
  MAINT: fix mistake in doc upload rule
  TST: attempt to make test_load_refcount deterministic
  BUG: Fix for numpy#6569, allowing build_ext --inplace
  TST: Added regression test empty percentile, in ref to numpy#6530 and numpy#6553
  TST: Added tests for empty partition and argpartition
  BUG: revert view safety checks
  TST: Remove tests of view safety checks (see next commit)
  BUG: Revert some import * fixes in f2py.
  BUG: Fixed partition errors on empty input. Closes numpy#6530
  DOC: import "numpy for matlab users" from the wiki
  DOC: reorganize user guide a bit + import "tentative numpy tutorial" from wiki
  DOC: remove placeholders and incompleteness warnings
  MAINT: minor update to "make upload" doc build command.
  BUG: error in broadcast_arrays with as_strided array
  BUG: fix inner() by copying if needed to enforce contiguity
  DOC: clarify usage of 'argparse' return value.
  BUG: Make median work for empty arrays (issue numpy#6462)
  ...
jaimefrio pushed a commit to jaimefrio/numpy that referenced this pull request Mar 22, 2016
@yashmehrotra @jaimefrio
TST: Added regression test empty percentile, in ref to numpy#6530 and n…
d66d0a2 
…umpy#6553
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
00 - Bug component: numpy._core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@yashmehrotra @charris @jaimefrio @njsmith