Skip to content

BUG: Make bool(void_scalar) and void_scalar.astype(bool) consistent #9856

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.

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

Merged
merged 1 commit into from
Oct 14, 2017

Conversation

eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Oct 12, 2017

Previously the behaviour of void.astype(bool) was:

  • Python 2:
    • itemsize == 0 - False
    • itemsize > 0 - True
  • Python 3:
    • itemsize == 0 - False, warn about casting empty arrays to bool
    • itemsize == 1 - True iff the sole element is 0
    • itemsize > 1- ValueError: setting an array element with a sequence.

Compared to the behaviour of bool(void), which is:

  • True iff all bytes are zero

The problem is that y = x.astype(bool) is implemented as y[i] = bool(x[i].item())

This patch changes it to use y[i] = bool(x[i]).

void.item() returns different results on python 2 and 3, which causes the crash.

IMO, item() is not something we should ever be using internally.

Works towards #9847


This is a less objectionable subset of #9848 with no compatibility ramifications

@charris
Copy link
Member

charris commented Oct 14, 2017

I get slightly different results with Python 2.7 and 1.14-dev

In [13]: void(3).astype(bool)
Out[13]: True

In [14]: void(2).astype(bool)
Out[14]: True

In [15]: void(1).astype(bool)
Out[15]: True

In [16]: void(0).astype(bool)
Out[16]: False

...

In [23]: void('123').astype(bool)
Out[23]: True

In [24]: void('12').astype(bool)
Out[24]: True

In [25]: void('1').astype(bool)
Out[25]: True

In [26]: void('').astype(bool)
Out[26]: False

So always True unless empty, no warnings.

EDIT: Same with void array scalars.

@charris
Copy link
Member

charris commented Oct 14, 2017

Also differs for Python 3.

@@ -1203,6 +1203,37 @@ def test_count_nonzero_unaligned(self):
a[:o] = False
assert_equal(np.count_nonzero(a), builtins.sum(a.tolist()))

def _test_cast_from_flexible(self, dtype):
Copy link
Member

Choose a reason for hiding this comment

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

Might want to document that this only tests scalars and scalar arrays.

@charris
Copy link
Member

charris commented Oct 14, 2017

So the rule in all these cases will be that 0D arrays and scalars containing empty byte strings, empty unicode strings, and V scalars converted to boolean yield False, and all other cases yield True. I assume this extends consistently to higher dimensional arrays.

@eric-wieser
Copy link
Member Author

Turns out I mispoke about python 2, and hadn't tested void(0).

Also differs for Python 3.

Can you elaborate on this?

@eric-wieser
Copy link
Member Author

eric-wieser commented Oct 14, 2017

So the rule in all these cases will be that 0D arrays and scalars containing empty byte strings, empty unicode strings, and V scalars converted to boolean yield False,

Yes. This was already true when converting with bool(...). The rule for void is "all bytes are zero" → False.

There's nothing special about 0d here, other than the fact that the LHS of the test bool(x) == x.astype(bool) is meaningful

@eric-wieser
Copy link
Member Author

eric-wieser commented Oct 14, 2017

Actually, the above is what the previous patch did. This patch only affects the visible behaviour of void scalars, and leaves strings alone

@charris
Copy link
Member

charris commented Oct 14, 2017

This seems correct to me, but will need a release note.

@charris
Copy link
Member

charris commented Oct 14, 2017

Can you elaborate on this?

I get ValueError: setting an array element with a sequence., but may be doing something different.

This patch only affects the visible behaviour of void scalars

Yep, but there are tests waiting to be enabled for the others.

@eric-wieser
Copy link
Member Author

I get ValueError: setting an array element with a sequence.

You're right, I had a different execution path in my head. I've updated the top comment with correct information.

Caused by void scalars decaying to 1d uint8 arrays before casting - `getitem` is dangerous for intermediate results

Works towards numpy#9847
@eric-wieser
Copy link
Member Author

Release note added

@charris charris merged commit d4afa3e into numpy:master Oct 14, 2017
@charris
Copy link
Member

charris commented Oct 14, 2017

Thanks Eric.

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

Successfully merging this pull request may close these issues.

2 participants