Code cleanup with pylint

[jngrad]

Follow-up to #3194

Description of changes:

• remove unused imports
• rewrite wildcard imports as explicit imports
• replace mutable optional parameters by immutable equivalents
• fix LB thermostat checkpointing mechanism
• cleanup and simplify conditional statements
• refactor code with numpy and new espresso methods
• remove unused variables/arguments
• rename unused loop variables from i to _
• fix broken numpy commands in parts of the testsuite that aren't executed
• remove trailling whitespaces

[codecov]

Codecov Report

Merging #3203 into python will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           python   #3203   +/-   ##
======================================
  Coverage      85%     85%           
======================================
  Files         530     530           
  Lines       25795   25795           
======================================
  Hits        22168   22168           
  Misses       3627    3627

---

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 94d51f5...1ef1aeb. Read the comment docs.

[KaiSzuttor]

are you sure we want to defer to 5.0? I think backporting bugfixes after the 4.1 release could be harder...

[RudolfWeeber]

Please defer. Otherwise all tutorials have to be rechecked

[jngrad]

Yes we should defer it to espresso 5. The bugs found here are not executed in our CI pipeline otherwise we would have got syntax errors. I've also not fixed bugs that require more than a one-liner change.

In espresso 4.1, the sources of syntax errors and runtime errors are:

src/python/espressomd/MDA_ESP/__init__.py:194:18:
    E0602: Undefined function 'triclinic_vectors()'
src/config/defines.py:24:4: W0102:
    Dangerous default value [] as argument
testsuite/python/data/gen_coulomb_2d_ref_data.py:18:0:
    W0622: Redefining built-in 'int', 'float', 'object', 'sum', 'any', 'range', ...

edit: fixes for the following bugs have been backported to 4.1 in f8e36f0:

testsuite/python/observables.py:81:32:
    E0602: Undefined function 'average()'
testsuite/python/observables.py:83:32:
    : Incorrect call to function 'sum()'
testsuite/python/tests_common.py:87:0:
    W0102: Dangerous default value [] as argument
testsuite/python/virtual_sites_tracers_common.py:37:4:
    W0102: Dangerous default value [] as argument

[RudolfWeeber]

I'm not opposed to the code changes in this PR.
I'd like to make a general remark though.
I think, the benefit of code beautification not done by automated formatting scripts needs to be weighed really carefully against the hassle caused in terms of merge conflicts.
I'm in favor of removing dangerous stuff such as wildcard imports into the current namespace.
In my opinion, stuff like isinstance refactoring or import sorting does not provide enough benefit, though.
The fact that something is in the pep rules doesn't necessarily mean following the rules is the best use of limited develpoer time.

This is particularly so for future autmated lint checking in PRs. There, rules should be added on an individual basis, only.

[mkuron]

I'm in favor of removing dangerous stuff such as wildcard imports into the current namespace.
In my opinion, stuff like isinstance refactoring or import sorting does not provide enough benefit, though.

I agree. New formatting rules should have limited impact on existing code. However, the changes in this pull request are rather small, increase code legibility and unlikely to cause major merge conflicts. So I think it should be merged.

[jngrad]

@RudolfWeeber thanks for your feedback! Here is my perspective:

• rules should be added on an individual basis: I strongly support this idea. Hopefully the dedicated issue Python3 linter #3194 will attract feedback that can be used in es meetings to decide on the rules to enforce. Whenever the C++ core is refactored/cleaned-up based on compiler warnings and Clang-format / Clang-tidy rules, we add those to the CMake logic to prevent the re-introduction of bad code. We can do the same with Python, progressively if need be. In Python3 linter #3194 (comment) I break down which of the pylint rules used in this PR should be enforced, and which ones shouldn't.
• import sorting: I have no strong opinion on whether to enforce it or not. We already enforce it automatically in C++, and it can be done automatically in Python too, so there shouldn't be any loss of developer time if we choose to do it. We should just make sure that espressomd.has_features don't get moved below the import statements, that parenthesis imports don't get transformed to a list of single-import statements and that pylint comments don't get removed.
• refactoring isinstance and friends: there are several spots in the code, especially in the testsuite, where the same code logic was used despite being inefficient, longer and less legible that the refactored version. Most likely these came from copy-pasting blocks of code "that just work". This is one aspect of technical debt. On quite a few occasions I lost time figuring out the intent of the original author in convoluted code blocks, only to realize from the git history it was just the result of minor fixes piling up in the same spot.
• code indentation and formatting: these ones cannot be enforced, since pylint does not apply them automatically. They should be done by autopep8, unfortunately it is not a perfect tool. My plan is to manually apply pylint formatting rules to the files already touched by fix_style.sh when creating a commit named "Formatting", so as to avoid wasting time in code reviews.
• merge conflicts: I agree it's a major issue with such large scale refactors. I'm not a big fan of them, and whenever I do one, I locally merge the most recent PRs to detect merge conflicts, and either reduce the scope of my refactor to exclude the conflicting files, or if I can't, communicate about it in the es meetings (ex: "if any merge conflict occurs in PRs, @jngrad can help" in Vector3d refactor, August 6). Sometimes conflicts go under the radar, recently a bors merge failed because of an extra newline character, and a pointer refactor caused conflicts in your PR and introduced a regression in a lambda. Managing large scale refactors is difficult and time-consuming. At the same time, we need them to improve the global state of espresso. Large-scale formatting PRs like this one are the worst in terms of added value divided by conflict resolving time, that's why it is best to do them after a code freeze. Most of the work has been done, now we just have to select a small subset of those rules we think should be enforced. The other rules can be checked by maintainers periodically, because the issues they detect tend to concentrate in poorly maintained parts of the code like OIF.

[KaiSzuttor]

maybe we should add a .pylintrc after the discussion is finished.

[jngrad]

I suggest we merge this PR in priority after the end of the summer school.

[jngrad]

@KaiSzuttor @christophlohrmann the NPT thermostat checkpointing might be broken too:

espresso/src/python/espressomd/thermostat.pyx

Lines 108 to 110 in 9dc1e06

	if thmst["type"] == "NPT_ISO":
	self.set_npt(kT=thmst["kT"], p_diff=thmst["p_diff"],
	piston=thmst["piston"])

does not match the function signature:

espresso/src/python/espressomd/thermostat.pyx

Line 419 in 9dc1e06

def set_npt(self, kT=None, gamma0=None, gammav=None):

[jngrad]

alright, fixing NPT revealed other bugs in the checkpointing mechanism for integrators. I'll have to open a separate PR, here is not the right place.

[jngrad]

The checkpointing bugfixes have their own PR now. The only relevant bugfix commit left in this PR for 4.1.1 (3b716e5) was cherry-picked in the release candidate. This is now ready for review.

[jngrad]

bors r=KaiSzuttor

[bors]

Build succeeded

• ICP GitLab CI