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

'Cause PEP-8 never goes out of style #558

Merged
merged 7 commits into from
Nov 13, 2023

Conversation

alex-rakowski
Copy link
Collaborator

pep 8 - Comparisons to singletons compliance

# goes from 
if arg == True:
    pass
if False == arg:
    pass
# to
if arg is True:
    pass
if arg is False:
    pass

Copy link
Member

@sezelt sezelt left a comment

Choose a reason for hiding this comment

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

For builtin Python booleans, testing == False and is False have the same behavior, but for other types this is not the case. In particular for numpy arrays, these do entirely different things: is compares the identity of the array object itself, while == is an elementwise value comparison. For example:

In [1]: import numpy as np

In [2]: ar = np.random.rand(3,3)

In [3]: ar
Out[3]:
array([[0.12607551, 0.56005212, 0.42254926],
       [0.48698435, 0.5986023 , 0.75294198],
       [0.82027795, 0.58230241, 0.92049683]])

In [4]: b = ar > 0.5

In [5]: b
Out[5]:
array([[False,  True, False],
       [False,  True,  True],
       [ True,  True,  True]])

In [6]: b is False
Out[6]: False

In [7]: b == False
Out[7]:
array([[ True, False,  True],
       [ True, False, False],
       [False, False, False]])

There is also one place where I can't tell the original meaning of the code, but from context it looks like it should actually be an assignment (=) rather than an equality test (see review comment).

py4DSTEM/process/diffraction/crystal_ACOM.py Outdated Show resolved Hide resolved
py4DSTEM/process/diffraction/crystal_ACOM.py Outdated Show resolved Hide resolved
py4DSTEM/process/fit/fit.py Outdated Show resolved Hide resolved
py4DSTEM/process/polar/polar_peaks.py Outdated Show resolved Hide resolved
py4DSTEM/process/strain/latticevectors.py Outdated Show resolved Hide resolved
py4DSTEM/process/strain/strain.py Outdated Show resolved Hide resolved
py4DSTEM/process/strain/strain.py Outdated Show resolved Hide resolved
py4DSTEM/process/strain/strain.py Outdated Show resolved Hide resolved
py4DSTEM/process/strain/strain.py Outdated Show resolved Hide resolved
@alex-rakowski
Copy link
Collaborator Author

Thanks @sezelt

Copy link
Member

@sezelt sezelt left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. I think we are just waiting on clarification of that one line whose original meaning was unclear

@sezelt sezelt self-requested a review November 10, 2023 21:04
Copy link
Member

@sezelt sezelt left a comment

Choose a reason for hiding this comment

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

Just needs clarification on the line in fit.py

@bsavitzky
Copy link
Member

So this is just changing every boolean == to an is? Not sure I'm into it :p

@sezelt
Copy link
Member

sezelt commented Nov 13, 2023

From PEP-8:

Comparisons to singletons like None should always be done with is or is not, never the equality operators.

See also this Stackoverflow:
https://stackoverflow.com/questions/27276610/boolean-identity-true-vs-is-true

This thread actually shows an interesting example where a numpy function that returns a scalar boolean does not return a Python singleton, and thus would behave differently under this change.

@alex-rakowski
Copy link
Collaborator Author

It changes == into is for comparisons to singletons, which is the pep-8 style.

Comparisons to singletons like None should always be done with is or is not, never the equality operators.
Also, beware of writing if x when you really mean if x is not None – e.g. when testing whether a variable or argument that defaults to None was set to some other value. The other value might have a type (such as a container) that could be false in a boolean context!

@sezelt caught the examples I incorrectly changed, which were used in numpy masks.

@alex-rakowski
Copy link
Collaborator Author

@sezelt @bsavitzky Changed to assignment should be good to go

@bsavitzky bsavitzky merged commit cf6a6a4 into py4dstem:dev Nov 13, 2023
7 checks passed
This was referenced Nov 15, 2023
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.

3 participants