-
Notifications
You must be signed in to change notification settings - Fork 25
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
Lint repo with ruff
#202
Lint repo with ruff
#202
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this, James! There are some changes that I am not sure about. Could you please take a look?
giddy/__init__.py
Outdated
from . import rank | ||
from . import util | ||
from . import sequence | ||
from . import directional # noqa F401, E402 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does # noqa F401, E402 mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are codes telling ruff
that those lines are not problems:
Code | Name | Message |
---|---|---|
F401 | unused-import | {name} imported but unused; consider using importlib.util.find_spec to test for availability |
E402 | module-import-not-at-top-of-file | Module level import not at top of file |
giddy/ergodic.py
Outdated
@@ -398,7 +401,7 @@ def var_mfpt_ergodic(p): | |||
k = P.shape[0] | |||
A = _steady_state_ergodic(P) | |||
A = np.tile(A, (k, 1)) | |||
I = np.identity(k) | |||
I = np.identity(k) # noqa E741 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does # noqa E741 mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code | Name | Message |
---|---|---|
E741 | ambiguous-variable-name | Ambiguous variable name: {name} |
giddy/markov.py
Outdated
if not hasattr(self, "_mfpt"): | ||
self._mfpt = mfpt(self.p, fill_empty_classes=True) | ||
return self._mfpt | ||
# @property |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mfpt()
is defined twice there, which triggers a ruff
error:
giddy/markov.py:292:9: F811 Redefinition of unused `mfpt` from line 285
Also, I am a bit confused on the need for two identical methods. Should one be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I will make the change and add to the PR. Thank you!
giddy/directional.py
Outdated
@@ -349,10 +350,10 @@ def plot_origin(self): # TODO add attribute option to color vectors | |||
Plot vectors of positional transition of LISA values starting | |||
from the same origin. | |||
""" | |||
import matplotlib.cm as cm | |||
# import matplotlib.cm as cm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For all commented out lines that I added, they were raising linting errors in ruff
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They can actually be entirely removed, but I decided to simply comment out for now so we can discuss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I think these lines are redundant and we should simply remove them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So these lines aren't actually redundant, it's just that import matplotlib.cm as cm
isn't used. That goes for all other commented out lines in this PR (except for L284-289 in markov.py
).
Only linting. No changes in functionality
This PR follows #201 as a step toward #200. There will be more linting to do once I get started in #200, but this is a good start.
@weikang9009 Another PR ready for review.