-
Notifications
You must be signed in to change notification settings - Fork 22
linting #180
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
linting #180
Conversation
…oken link in the notebooks readme, did a ton of linting, realized we don't really need linting.py and that a lot of stuff i nthe .pylintrc was out of date
| @@ -1,5 +1,3 @@ | |||
| # pylint: skip-file | |||
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.
Files can be skipped via directive in the .pylintrc file, so I moved the instruction there.
| - **dxdt_hat** (np.array) -- estimated derivative of x | ||
| """ | ||
| if params != None: # Warning to support old interface for a while. Remove these lines along with params in a future release. | ||
| if params is not None: # Warning to support old interface for a while. Remove these lines along with params in a future release. |
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.
Using == rather than is to check Noneness was probably my most common sin, according to the linter.
| cutoff = np.sqrt(-2 * sigma**2 * np.log(1e-4)) | ||
| rows, cols, vals, dvals = [], [], [], [] | ||
| for n in range(len(t)): | ||
| for n in range(len(t)): # pylint: disable=consider-using-enumerate |
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.
Every so often I slotted in a directive to ignore pylint's desires. This one I like better this way, because you need to have access to and record n anyway, and t[n] has a visual rhyme with t[j].
| @@ -1,8 +1,7 @@ | |||
| """This module implements constant-derivative model-based smoothers based on Kalman filtering and its generalization.""" | |||
| import numpy as np | |||
| from warnings import warn | |||
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.
The linter wanted this builtin import to happen before external imports, even numpy. I'm okay with that, but it's funny I see numpy as even more foundational. Python would be nothing without it; it's the one package to rule them all. I might consider ignoring this silly import order thing, but I've already changed over everyone, so whatever.
| return _polydiff(*args, **kwargs) | ||
|
|
||
| def spectraldiff(*args, **kwargs): # pragma: no cover | ||
| def spectraldiff(*args, **kwargs): # pragma: no cover pylint: disable=missing-function-docstring |
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.
Passthrough stubs don't need docstrings. They'll die soon anyway.
| results += pool.map(_minimize, starting_points) # returns a bunch of OptimizeResult objects | ||
| else: # For experiments, where I want to parallelize optimization calls and am not allowed to have each spawn further processes | ||
| cache = dict() | ||
| cache = {} # dict |
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 some reason I don't fully understand, {} is preferred to dict() by the linter. I gather the former evaluates to a literal, while the latter is a function call that returns a literal. Why would one be less direct than the other? I've historically preferred dict() because {} looks to me like the empty set, especially because type({'a'}) prints <class 'set'>.
| @@ -1,7 +1,6 @@ | |||
| """Apply smoothing method before finite difference.""" | |||
| import numpy as np | |||
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.
Funny I manage not to make any direct numpy calls in this whole file.
| duplicate-code, | ||
| invalid-name, | ||
| invalid-unary-operand-type, | ||
| print-statement, |
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.
Most of these were giving me warnings from the linter that they're no longer valid rules to check. Happy to simplify and to shorten the list we're excluding/ignoring/disabling.
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.
This thing was so written by AI. I made it warmer and less repetitive.
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.
Calling python3 linting.py pynumdiff does the same thing as python3 -m pylint pynumdiff, so unclear what the value add of this file is. Getting a zero or nonzero exit code back could be handy if you wanted to call pylint from CI and make jobs fail if they don't exceed the threshold score, but we're not calling pylint from our CI, and I'm not sure we need to. Linting feels like a "do it every once in a while and just don't let it get too out of control" sort of thing, not like tests which need to pass every time. There is also a fail-under threshold in the .pylintrc file, which makes me think there is a way to get exit codes from the -m module call should we need them. May as well simplify the repo where possible.
greatly shortened and deduplicated the contributing guide, fixed a broken link in the notebooks readme, did a ton of linting, realized we don't really need linting.py and that a lot of stuff i nthe .pylintrc was out of date