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

Code Linting with Pylint #150

Merged
merged 45 commits into from
Aug 19, 2020

Conversation

nathanaelbosch
Copy link
Collaborator

@nathanaelbosch nathanaelbosch commented Aug 18, 2020

Closes #108 (and relates to #111).

Current state of the PR
I created configs, added a github action, and started fixing many errors. However I did not fix all issues, and often chose to add exceptions. I think that the best way would be to discuss these exceptions, fix those that should absolutely go into this PR, and then open specific issues about individual points.

Commentary for each of the exceptions:

  • invalid-name: We will probably want to keep this, since we often want to use mathy notation
  • no-else-return, no-else-raise: Not so clear to me what the benefit of fixing this is
  • fixme: Can be resolved through the separate issue Remove TODOs or change them to correct format #69
  • too-many-arguments: Appears accross modules. Good code style improvement, but not crucial. We could also raise the number of arguments from to 6 or 7 to resolve most warnigns, and to put the focus towards the cases with 12 (distribution.py) and 10 (linearsolvers.py) arguements.
  • missing-module-docstring, missing-function-docstring: Could be considered as part of Automatically check whether docstrings are complete and correctly formatted #95, or we can also create a new issue. Although I have the impression that noone will do this at once.
  • abstract-method: Needs to be fixed in probnum.linalg and probnum.prob
  • unused-argument: Many of these are cases where **kwargs gets passed around. I had the impression that this is often the "standard" in our code so I did not go ahead and fix it.
  • too-many-branches: Only affects linearsolves.py and matrixbased.py. I'd just open an issue for this specific point.
  • arguments-differ: Multiple cases in linalg, but also appearsx in prob, quad and diffeq
  • redefined-builtin: In quad and linalg; Fixing this will change some function signatures
  • too-few-public-methods: Mostly quad, but also diffeq.ODESolver; Suggests considering removing these classes
  • too-many-locals: Seems like a good tool to improve code, but is also non-trivial to fix
  • no-self-use: Suggests making functions instead of methods
  • too-many-lines: Suggests refactoring matrixbased
  • broad-except: Only appears in normal.py. Should be easier to fix for someone familiar with that code.
  • abstract-class-instantiated: Only appears in diffeq due to Nico's code structure; I think it would be best if he has a look.
  • protected-access: Something we probably want, but it's also not trivial or fast to fix
  • duplicate-code: Only appears in filtsmooth
  • too-complex: McCabe complexity checker; Complains for matrixbased
  • too-many-instance-attributes: Indicated that matrixbased.py and distribution might need some refactoring
  • too-many-statements: Again in matrixbased
  • attribute-defined-outside-init: Mostly in matrixbased. One appearance in bayesquadrature.py, but that code seems empty anyways.

More exceptions for tests:
duplicate-code, line-too-long, missing-class-docstring, unnecessary-pass. I did not fix them becasue it did not feel too important for now, but especially the line-too-long and unnecessary-pass should be easy to fix quite mechanically. Also unused-variable, but in tests we often want to compute some value without actually using it afterwards, so that seems reasonable.

Next steps
I think it would be good to merge a first version as soon as possible, and just open issues for the parts that we want to have fixed.
But it would be good to know if you think that we should still resolve one of the current exceptions in this PR.
If we decide to merge, I'll open individual issues for the remaining points.

Also, if you have any suggestion on how to better include pylint into our CI let me know!

@nathanaelbosch nathanaelbosch marked this pull request as ready for review August 18, 2020 09:07
@codecov
Copy link

codecov bot commented Aug 18, 2020

Codecov Report

Merging #150 into master will increase coverage by 0.05%.
The diff coverage is 62.06%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #150      +/-   ##
==========================================
+ Coverage   73.26%   73.32%   +0.05%     
==========================================
  Files          57       57              
  Lines        3056     3040      -16     
  Branches      394      391       -3     
==========================================
- Hits         2239     2229      -10     
+ Misses        665      660       -5     
+ Partials      152      151       -1     
Impacted Files Coverage Δ
src/probnum/__init__.py 84.61% <ø> (ø)
src/probnum/diffeq/__init__.py 100.00% <ø> (ø)
src/probnum/diffeq/ode/__init__.py 100.00% <ø> (ø)
src/probnum/diffeq/ode/ivp.py 98.57% <ø> (ø)
src/probnum/diffeq/ode/ode.py 100.00% <ø> (ø)
src/probnum/diffeq/odefiltsmooth/ivpfiltsmooth.py 93.15% <ø> (-0.10%) ⬇️
src/probnum/diffeq/odefiltsmooth/prior.py 95.04% <ø> (ø)
...m/filtsmooth/gaussfiltsmooth/unscentedtransform.py 96.00% <ø> (ø)
...iltsmooth/statespace/continuous/continuousmodel.py 92.59% <0.00%> (ø)
...filtsmooth/statespace/continuous/linearsdemodel.py 83.80% <ø> (ø)
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0b94f0...7c282b3. Read the comment docs.

@JonathanWenger
Copy link
Contributor

In general, I think all these issues raised should be fixed (obviously) but none in this PR specifically. For some there are already issues (e.g. for linalg: #51) and some might be fixed by larger changes in progress at the moment (e.g. #83, #131). In summary I agree this PR should be merged and some of the points above moved into (existing) issues.

@nathanaelbosch
Copy link
Collaborator Author

A few things that should be done as a part of this PR:

  • perform the pylint check on a "per-module" basis. This allows us to check for things that are currently violated in one of the modules and make sure that we do not introduce new issues.
  • Open refactoring issues for the individual modules. Fixing these errors on a per-module basis is easier. Also, we can raise the standards after fixing each individual error, for each module separately.

@nathanaelbosch nathanaelbosch merged commit 1ab81a7 into probabilistic-numerics:master Aug 19, 2020
@nathanaelbosch nathanaelbosch deleted the pylint branch August 19, 2020 09:30
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.

Use flake8 to better verify PEP 8 compliance
2 participants