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

Replace NumPy's logical_and and logical_or #3115

Merged
merged 2 commits into from
Apr 12, 2024
Merged

Replace NumPy's logical_and and logical_or #3115

merged 2 commits into from
Apr 12, 2024

Conversation

michalhabera
Copy link
Contributor

Improves readability, especially for nested conditionals.

@jhale
Copy link
Member

jhale commented Mar 28, 2024

The new code is correct and easier to read. However I've seen users replacing logical operator with a bitwise operator as 'they do the same thing' before, with bad results!

@nate-sime
Copy link
Contributor

@jhale do you have an example of where you'd get an unexpected result? I've been using the style introduced by this PR with no issues.

@jhale
Copy link
Member

jhale commented Mar 28, 2024

https://numpy.org/doc/1.26/user/numpy-for-matlab-users.html

Yes I've seen people attempt bitwise ops on integer arrays and expecting the logical op.

There are some additional pitfalls especially for users coming from MATLAB. My view is that np.logical_* is the correct expression of the programmer's intent in Python. It's unfortunate there isn't a operator version of the logical numpy ops.

I'm confident this change will cause issues/confusion for non-expert users.

@jhale
Copy link
Member

jhale commented Mar 28, 2024

I'm ok for one or two of the demos which needs this most to use this style, but please stick a comment there warning that this is the bitwise op!

@michalhabera
Copy link
Contributor Author

The PR uses | and & exclusively on boolean array, where bitwise and logical are equivalent. It is also hard to do something dangerous in our lambda markers, as bitwise ops work only on bool and int. So it would be better to show specific example where these ops are dangerous in the context of geometrical markers.
But whatever the decision is, I'd prefer a consistent approach.

@jhale
Copy link
Member

jhale commented Mar 28, 2024

Everyone in this thread understands that logical operations are equivalent with bitwise operations on arrays of bools.

But do users reading our introductory demos understand that? A Python callable being passed is already difficult conceptually for many.

logical_* is clear, explicit and always has correct meaning.

@jhale jhale added this pull request to the merge queue Apr 12, 2024
Merged via the queue into main with commit d52139f Apr 12, 2024
19 checks passed
@jhale jhale deleted the michal/numpy_logical branch April 12, 2024 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants