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.

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

ENH: Add bitwise_and identity #7373

Merged
merged 4 commits into from Mar 5, 2016
Merged

Conversation

charris
Copy link
Member

@charris charris commented Mar 2, 2016

Couple of simple fixes for the bitwise operators.

  • Give bitwise_xor the identity zero rather than none.
  • Give bitwise_and an identity of all bits set (-1).

Still needs tests, so do not merge.

The identity for bitwise_xor is zero.
@charris charris changed the title Add bitwise and identity Add bitwise_and identity Mar 2, 2016
@charris charris changed the title Add bitwise_and identity ENH: Add bitwise_and identity Mar 2, 2016
* UFunc has unit of 0, and the order of operations can be reordered
* This case allows reduction with multiple axes at once.
* UFunc has unit of -1, and the order of operations can be reordered
* This case allows reduction with multiple axes at once. Intended for
Copy link
Contributor

Choose a reason for hiding this comment

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

That last sentence is not strictly necessary. There could potentially be other ufuncs that use this.

Copy link
Member Author

Choose a reason for hiding this comment

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

There might be others, but I could not think of any. Hence the bit of explanation as to why this otherwise useless macro is there. I suppose it could be "Introduced" instead of "Intended".

@jaimefrio
Copy link
Member

Nice! Last week I went to a "Hacker Garten" event, and spent an hour or so introducing NumPy to one of the "hacker gardeners", and this came up in the "easy fix" issue list. We spent about 10 minutes seeing if it would be a good fit for his first PR, which it obviously wasn't.

@charris
Copy link
Member Author

charris commented Mar 3, 2016

For reference, the other option is #7061 which uses ReorderableNone as the identity. That should only make a difference for empty arrays. Another corner case to deal with is empty object arrays. Object arrays are already special cased:

            if (PyArray_ISOBJECT(arr) && PyArray_SIZE(arr) != 0) {
                assign_identity = NULL;
            }

We can choose what to use as the return value in that case, if any, or raise an error.

@seberg
Copy link
Member

seberg commented Mar 3, 2016

Hmmm forgot about that. I admit the object cases are always a little weird. But that is also the case for half the current identities. For non-object, to me this option is "obviously right".

@charris
Copy link
Member Author

charris commented Mar 3, 2016

Updated tests.

@charris
Copy link
Member Author

charris commented Mar 3, 2016

also might be good to add a test for bitwise_and.reduce for non-boolean inputs,

@madphysicist The ones isn't boolean, it is all bits set. We only need to check that everything works correctly bitwise for every bit, and that is done.

@madphysicist
Copy link
Contributor

Good point. I rescind my comment.

@charris
Copy link
Member Author

charris commented Mar 4, 2016

Testing with PyInt_FromLong to see what happens.

Current value is 1, which only works for the low order bit. Use -1
instead.

Closes numpy#7060.
- test values
- test identity for bitwise_or, bitwise_xor, bitwise_and
@charris
Copy link
Member Author

charris commented Mar 4, 2016

OK, works, so squashing that commit into others. The only open questions is what to do with empty object arrays. Because non-empty object arrays do not use the identity, it might be most consistent to raise an error in that case, but currently the identity is returned.

The identity has changed from 1 to -1.
@seberg
Copy link
Member

seberg commented Mar 4, 2016

Thanks. @jaimefrio since you already looked at it, do you have a preference whether we should -1 return for empty object arrays, or rather make the identity "crash" on object arrays if used? I can't make up my mind and either way has its downsides ;).

@njsmith
Copy link
Member

njsmith commented Mar 4, 2016

I guess I prefer erroring out on empty object arrays because "in the face of ambiguity refuse to guess". But it doesn't seem hugely important either way.

@charris
Copy link
Member Author

charris commented Mar 4, 2016

Note that erroring out will reguire a deprecation cycle, so is probably independent of this PR.

@seberg
Copy link
Member

seberg commented Mar 4, 2016

In that case, I guess we should put this in as a bug fix and then open an issue about the other stuff? Will merge in a bit if nothing crops up.

@seberg
Copy link
Member

seberg commented Mar 5, 2016

OK, thanks Chuck! Will open an issue about the object identity.

@xflr6
Copy link

xflr6 commented Mar 13, 2016

If this is finally resolved (great!) then I guess #4696 and #5250 can also be closed.

@charris
Copy link
Member Author

charris commented Mar 13, 2016

@xflr6 Thanks for the heads up.

@charris charris deleted the add-bitwise_and-identity branch September 19, 2018 14:38
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.

None yet

6 participants