Skip to content

MAINT: Use XOR for bool arrays in np.diff - #9259

Merged
charris merged 2 commits into
numpy:masterfrom
soupault:fix_9251
Jun 18, 2017
Merged

charris merged 2 commits into
numpy:masterfrom
soupault:fix_9251

Conversation

@soupault

@soupault soupault commented Jun 17, 2017 •

Copy link
Copy Markdown
Contributor

The binary - operator is no longer supported for booleans. Use xor instead to maintain backwards compatibility.

Added some tests for this case, and also for float.

Closes #9251.

Comment thread numpy/lib/function_base.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I somewhat prefer == np.bool_.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also it would be nice if you can make the commit use our typical format such as BUG: ... (or maybe MAINT: here even, sometimes its hard to say ;)).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@seberg should be good now.

@charris charris changed the title Use XOR for bool arrays in np.diff MAINT: Use XOR for bool arrays in np.diff Jun 17, 2017
Comment thread numpy/lib/function_base.py Outdated
slice2 = tuple(slice2)

if a.dtype == np.bool_:
da = a[slice1] ^ a[slice2]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I might be tempted to use !=?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Furthermore, if we use !=, then we can add support for dtypes with fields here too. (Possibly an argument for a different name)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

!= is kinda nice. Don't know if it add any funtionality. @eric-wieser Could you explain a bit more.

@soupault soupault Jun 17, 2017 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we can add support for dtypes with fields here too

@eric-wieser I remember that amazing feature I was always willing to try :). Is better to keep it for a separate PR though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I note that at the ufunc level, logical_xor is defined as not_equal for booleans, but the operator != goes through richcompare while ^ does not, hence ^ is a trifle faster.

One problem with the deprecation of - is that if we remove the boolean implementation the bools will be upcast to int8, which will really confuse people, but at least be consistent with python ...

@eric-wieser

eric-wieser commented Jun 17, 2017 •

Copy link
Copy Markdown
Member

Could you explain a bit more.

Test would become a.dtype == np.bool_ or a.dtype.fields, and would enable np.diff(struct_array), meaning"adjacent entries are different".

Of course, it would be weird to not also support this for float arrays, suggesting that the "adjacent entries are different" operation should be given a different name to diff, and diff(bool_array) should DeprecationWarning, recommending the new name.

Possible names for the new function could be:

  • np.not_equal.adjacent(a) - has the bonus that diff becomes a simple alias for np.subtract.adjacent
  • np.adjacent_different

@eric-wieser

Copy link
Copy Markdown
Member

I think that's outside the scope of this PR though, and should be considered after merging this

@charris
charris merged commit 1d9189e into numpy:master Jun 18, 2017
@charris

charris commented Jun 18, 2017

Copy link
Copy Markdown
Member

Thanks @soupault .

@soupault
soupault deleted the fix_9251 branch June 18, 2017 16:28
@jakirkham

Copy link
Copy Markdown
Contributor

Did this get backported into NumPy 1.13/land in the 1.13.1 release, @charris?

@charris

charris commented Jul 7, 2017

Copy link
Copy Markdown
Member

@jakirkham No, the - operator was reinstated with the same old DeprecationWarning.

@jakirkham

Copy link
Copy Markdown
Contributor

Ok, makes sense. Thanks.

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.

5 participants