8000 Fixed bug in numpy.piecewise() for 0-d array handling by ericsuh · Pull Request #331 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

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

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 3 commits into from
Closed

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

wants to merge 3 commits into from

Conversation

ericsuh
Copy link
@ericsuh 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
Fixed bug in numpy.piecewise()
e553e1b 
Updated numpy/lib/function_base.py to fix bug in numpy.piecewise() for 0-d
array handling and boolean indexing with scalars.
@travisbot
Copy link

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

@njsmith
Copy link
Member
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
Fixed numpy.piecewise() and tests
e2ad1a7 
Corrected tests for numpy.piecewise(), made them more systematic, and
altered numpy.piecewise fix to hit all test cases.
@ericsuh
Copy link
Author
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.

@travisbot
Copy link

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

@njsmith
Copy link
Member
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?

@astrojuanlu
Copy link
Contributor

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.

@njsmith
Copy link
Member
njsmith commented Nov 15, 2012

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

@astrojuanlu
Copy link
Contributor

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

@njsmith
Copy link
Member
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
Copy link
Author
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.

@astrojuanlu
Copy link
Contributor

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.

@njsmith
Copy link
Member
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//pull/331#issuecomment-10467386.

@charris
Copy link
Member
charris commented Mar 1, 2013

What is the status of this?

@astrojuanlu
Copy link
Contributor

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.

@astrojuanlu
Copy link
Contributor

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.

@njsmith
Copy link
Member
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//pull/331#issuecomment-14276274
.

@ericsuh
Reverted numpy.piecewise() tests to original set with regression test
c760734 
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.
@ericsuh
Copy link
Author
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.

@charris
Copy link
Member
charris commented Aug 16, 2013

@njsmith Can this go in.

@charris
8000
Copy link
Member
charris commented Aug 16, 2013

Ping Travisbot just because it was a long time ago.

@charris charris closed this Aug 16, 2013
@charris charris reopened this Aug 16, 2013
charris
charris reviewed Aug 18, 2013
View reviewed changes
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 \
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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//pull/331/files#r5831760
.

@charris
Copy link
Member
charris commented Aug 18, 2013

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

@charris
Copy link
Member
charris commented Aug 18, 2013

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

@charris
Copy link
Member
charris commented Feb 16, 2014

Needs finishing up.

@charris
Copy link
Member
charris commented Mar 28, 2014

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

@astrojuanlu
Copy link
Contributor

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):

numpy/numpy/lib/tests/test_function_base.py

Line 1468 in b1c69df

x = piecewise([0, 0], [True, False], [1])

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.

@astrojuanlu astrojuanlu mentioned this pull request Jun 7, 2014
Merged
@astrojuanlu
Copy link
Contributor

I added a new PR (see #4792).

luyahan pushed a commit to plctlab/numpy that referenced this pull request Apr 25, 2024
@howjmay
Merge pull request numpy#331 from howjmay/vsubl_high
3053e5f 
feat: Add vsubl_high_[s8|s16|s32|u8|u16|u32]
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
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ericsuh @travisbot @njsmith @astrojuanlu @charris
0