Fixed bug in numpy.piecewise() for 0-d array handling #331

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
5 participants

ericsuh commented Jul 5, 2012

Updated numpy/lib/function_base.py to fix bug in numpy.piecewise() for 0-d array handling and boolean indexing with scalars.

Bug test case:

>>> numpy.piecewise(5, [True, False], [1, 0])
Traceback (most recent call last):
    [...]
    y[condlist[k]] = item
ValueError: boolean index array has too many values

After fix:

>>> numpy.piecewise(5, [True, False], [1, 0])
array(1)
@ericsuh ericsuh Fixed bug in numpy.piecewise()
Updated numpy/lib/function_base.py to fix bug in numpy.piecewise() for 0-d
array handling and boolean indexing with scalars.
e553e1b

This pull request fails (merged e553e1b into 731cf3a).

Owner

njsmith commented Jul 6, 2012

The current version of this pull request appears to fail the existing tests (e.g., this build reports failures: http://travis-ci.org/#!/numpy/numpy/jobs/1788964), so that would need to be fixed.

Also, some sort of test needs to be added to make sure that the bug is fixed (and stays that way).

@ericsuh ericsuh Fixed numpy.piecewise() and tests
Corrected tests for numpy.piecewise(), made them more systematic, and
altered numpy.piecewise fix to hit all test cases.
e2ad1a7

ericsuh commented Jul 7, 2012

Hi njsmith, thanks. I've fixed both my change to numpy.piecewise() as well as the test cases to cover both my bug case above as well as making sure that the tests are all logical.

This pull request passes (merged e2ad1a7 into 731cf3a).

Owner

njsmith commented Jul 11, 2012

I guess you rearranged the order of all the tests in the source? It makes the diff unreadable unfortunately -- I can't tell at all what you've actually changed without reverse-engineering it by hand, and I'm afraid I'm too lazy for that. Can you revert the no-op changes, or split them into a separate commit, or at least describe what changes were actually made?

Contributor

Juanlu001 commented Nov 15, 2012

Is this issue (i.e. this pull request) going to be fixed anytime soon? It's a bit anoying to construct a 1d array with one element each time I want to call this function with a scalar.

Owner

njsmith commented Nov 15, 2012

I guess it will be fixed as soon as someone does the work :-)

Contributor

Juanlu001 commented Nov 15, 2012

Hm, got it :) Should an issue be filed first?

Owner

njsmith commented Nov 15, 2012

The main point of filing an issue is to make sure that something doesn't get forgotten, and this PR is already sort of doing that (i.e., it shows up on the list of open issues if anyone goes to look at it). But feel free to file one if you like. If you're thinking about fixing the bug yourself, then we're pretty laid back about these things -- you can just go ahead and submit a new PR without filing an issue if you want. We'd rather get the fix than worry about the process :-).

ericsuh commented Nov 15, 2012

The main thing that needs done is just to rewrite the unit tests. In my own testing and usage (and when I've tested with the original unit tests), the code works fine, but I just haven't had the time to sit down and rewrite them to be more similar to the original set and still be complete and systematic. I may get to it next week sometime, but until then I'm swamped.

-- Eric

On Nov 15, 2012, at 9:33 AM, njsmith notifications@github.com wrote:

The main point of filing an issue is to make sure that something doesn't get forgotten, and this PR is already sort of doing that (i.e., it shows up on the list of open issues if anyone goes to look at it). But feel free to file one if you like. If you're thinking about fixing the bug yourself, then we're pretty laid back about these things -- you can just go ahead and submit a new PR without filing an issue if you want. We'd rather get the fix than worry about the process :-).


Reply to this email directly or view it on GitHub.

Contributor

Juanlu001 commented Nov 17, 2012

That's one of the things I was about to ask, it is ok to rewrite the tests? Because right now IMHO they don't seem very meaningful for the purpose of the function. I'd like to try it myself this weekend, or next one as much.

Owner

njsmith commented Nov 17, 2012

It's OK to make any changes whatsoever, the requirements are just that you
should have a reason, and we should be able to look at your changes and
figure out if they're good :-). How exactly to do that depends on the
change. Easily readable diffs, English explanations of what you did and
why... whatever works.
On 17 Nov 2012 00:02, "Juan Luis Cano Rodríguez" notifications@github.com
wrote:

That's one of the things I was about to ask, it is ok to rewrite the
tests? Because right now IMHO they don't seem very meaningful for the
purpose of the function. I'd like to try it myself this weekend, or next
one as much.


Reply to this email directly or view it on GitHubhttps://github.com/numpy/numpy/pull/331#issuecomment-10467386.

Owner

charris commented Mar 1, 2013

What is the status of this?

Contributor

Juanlu001 commented Mar 1, 2013

Well actually I didn't do it any of the weekends as you can notice :( But anyway now that you ask about it, if I recall correctly there was some discussion recently about rank-0 arrays in the mailing list but I am not able to find it. Are there any constraints on how to do this? Other than that it would be my first contribution to NumPy and I am a bit unsecure but I am willing to give it a try.

Contributor

Juanlu001 commented Mar 1, 2013

Yes, I was referring to this:

http://www.mail-archive.com/numpy-discussion@scipy.org/msg39968.html

My question was if there are any near future plans for rank-0 arrays that relate this and other issues in such a way that it requires changing low-level code or if we can address this individually.

Owner

njsmith commented Mar 1, 2013

No, I don't think there will be any near-future changes in how 0d arrays
work that will affect this bug.
On 1 Mar 2013 07:29, "Juan Luis Cano Rodríguez" notifications@github.com
wrote:

Yes, I was referring to this:

http://www.mail-archive.com/numpy-discussion@scipy.org/msg39968.html

My question was if there are any near future plans for rank-0 arrays that
relate this and other issues in such a way that it requires changing
low-level code or if we can address this individually.


Reply to this email directly or view it on GitHubhttps://github.com/numpy/numpy/pull/331#issuecomment-14276274
.

@ericsuh ericsuh Reverted numpy.piecewise() tests to original set with regression test
Fixed one bug in fix, changed unit tests of numpy.piecewise() to be the
original set except for the addition of one regression test.

Revisions of unit tests to include spelling out undocumented assumptions
of behavior of numpy.piecewise() will come later.
c760734

ericsuh commented Mar 1, 2013

Sorry for the delay of months. Finishing up my degree and job hunting, so I've been...occupied. I reverted most of the unit test changes so that it's clearer what I changed. Maybe I'll re-up the unit test changes at some point, but this particular commit should be easier to understand when you diff between this state and the state 3 commits ago.

Owner

charris commented Aug 16, 2013

@njsmith Can this go in.

Owner

charris commented Aug 16, 2013

Ping Travisbot just because it was a long time ago.

charris closed this Aug 16, 2013

charris reopened this Aug 16, 2013

@charris charris commented on the diff Aug 18, 2013

numpy/lib/function_base.py
@@ -674,20 +674,13 @@ def piecewise(x, condlist, funclist, *args, **kw):
x = asanyarray(x)
n2 = len(funclist)
if isscalar(condlist) or \
- not (isinstance(condlist[0], list) or
- isinstance(condlist[0], ndarray)):
+ (isinstance(condlist, np.ndarray) and condlist.ndim == 0) or \
@charris

charris Aug 18, 2013

Owner

This changes the behavior and needs to be documented. I've also looked at the following version:

    def isscalarlike(a):
        return isscalar(a) or (isinstance(a, np.ndarray) and a.ndim == 0)

    x = asanyarray(x)
    n2 = len(funclist)
    if isscalarlike(condlist) or (x.ndim > 0 and isscalarlike(condlist[0])):
        condlist = [condlist]

Which is a bit different again, but clearer I think.

@njsmith

njsmith Aug 18, 2013

Owner

Yeah, I suspect this change is probably fine but I haven't wrapped my head
around that complex conditional...

On Sun, Aug 18, 2013 at 5:01 PM, Charles Harris notifications@github.comwrote:

In numpy/lib/function_base.py:

@@ -674,20 +674,13 @@ def piecewise(x, condlist, funclist, _args, *_kw):
x = asanyarray(x)
n2 = len(funclist)
if isscalar(condlist) or \

  •       not (isinstance(condlist[0], list) or
    
  •            isinstance(condlist[0], ndarray)):
    
  •        (isinstance(condlist, np.ndarray) and condlist.ndim == 0) or \
    

This changes the behavior and needs to be documented. I've also looked at
the following version:

def isscalarlike(a):
    return isscalar(a) or (isinstance(a, np.ndarray) and a.ndim == 0)

x = asanyarray(x)
n2 = len(funclist)
if isscalarlike(condlist) or (x.ndim > 0 and isscalarlike(a[0])):
    condlist = [condlist]

Which is a bit different again, but clearer I think.


Reply to this email directly or view it on GitHubhttps://github.com/numpy/numpy/pull/331/files#r5831760
.

@charris charris commented on the diff Aug 18, 2013

numpy/lib/function_base.py
@@ -704,6 +697,16 @@ def piecewise(x, condlist, funclist, *args, **kw):
newcondlist.append(condition)
condlist = newcondlist
+ if n == n2-1: # compute the "otherwise" condition.
@charris

charris Aug 18, 2013

Owner

Might as well put spaces around the - for PEP8 compliance.

Owner

charris commented Aug 18, 2013

@ericsuh Made some comments. The commits should also be squashed into one.

Owner

charris commented Aug 18, 2013

I've put up a cleaned version at https://github.com/charris/numpy in the branch gh-331

Owner

charris commented Feb 16, 2014

Needs finishing up.

charris added the Easy Fix label Feb 16, 2014

Owner

charris commented Mar 28, 2014

I seem to have deleted my cleanup somewhere along the line.

Contributor

Juanlu001 commented Jun 1, 2014

In the docstring:

"Each boolean array in condlist selects a piece of x, and should therefore be of the same shape as x."

But actually even the tests do not respect this (and still everything works):

https://github.com/numpy/numpy/blob/b1c69df01b673cc086065112da6780d8548a0dfa/numpy/lib/tests/test_function_base.py#L1468

There are other bugs too. For instance, there's test_0d but even though np.piecewise(x, x > 3, [4, 0]) passes, np.piecewise(x, [x > 3, x <= 3], [4, 0]) fails (same error as originally reported). I don't think this behaviour is consistent - that's what I actually meant a year and a half ago. The precondition stated in the docstring makes sense for me and should be somehow enforced - is anybody against it? I cannot promise anything but I may try and solve this at once.

@Juanlu001 Juanlu001 added a commit to Juanlu001/numpy that referenced this pull request Jun 7, 2014

@Juanlu001 Juanlu001 Added another 0d test
This is to reflect that the original PR #331 is actually
a non-issue because "each boolean array in `condlist`
selects a piece of `x`, and should therefore be of the
same shape as `x`". The correct way is as just added.
974df99

@Juanlu001 Juanlu001 added a commit to Juanlu001/numpy that referenced this pull request Jun 7, 2014

@Juanlu001 Juanlu001 Added another 0d test
This is to reflect how should 0d conditions be specified without
shortcuts (see PR #331).
1cce6e4

@Juanlu001 Juanlu001 added a commit to Juanlu001/numpy that referenced this pull request Jun 7, 2014

@Juanlu001 Juanlu001 Changed piecewise body, fixes #331
As seen in test_simple, when `x` has more than one element
the condlist `[True, False]` is being made equivalent to
`[[True, False]]`, which is correct. However, when `x` is
zero dimensional the expected condlist is `[[True], [False]]`
(see 1cce6e4). Therefore, when a 0d array is given the transpose
is used.
9fa4b59

Juanlu001 referenced this pull request Jun 7, 2014

Merged

Piecewise fix for 0d #4792

Contributor

Juanlu001 commented Jun 7, 2014

I added a new PR (see #4792).

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