Long-term global pylint ignores

[marvinpfoertner]

In order to close #469, we should discuss which global pylint ignores should be retained long-term.
The results of the discussion should be summarized concisely in this first post.

Long-term pylint ignores

bad-continuation
bad-whitespace
invalid-name
fixme

Additional long-term pylint ignores for the tests

missing-function-docstring

Additional custom pylint configuration

• raise too-many-arguments threshold from 5 (default) to 10
• raise too-many-locals threshold from 15 (default) to 20, since arguments count as locals
• temporary ignore of missing-return-doc and missing-yield-doc until a missing feature in numpydoc, pylint, and sphinx.ext.napoleon is implemented (see this thread)

We can leverage the comment threads to have one structured thread for each potential ignore.
Now discuss :) @JonathanWenger @pnkraemer @mmahsereci @alpiges

[marvinpfoertner]

bad-continuationand bad-whitespace should stay, because they are suggested by black to make the autoformatted code pylint compatible

[marvinpfoertner]

I believe that we decided to ignore invalid-name because we want to be able to name e.g. matrix variables as A

[marvinpfoertner]

We decided that we would ignore fixme because we want to allow clearly formatted TODOs like

# TODO (#[issue number]): This is what needs to be done

[marvinpfoertner]

It makes sense to keep redefined-variable-type because we often change the type of a variable when normalizing a function argument (e.g. casting a np.ndarray into a LinearOperator using pn.linops.aslinop or casting a float into a np.double using pn.utils.as_numpy_scalar). Changing the variable names in these cases would lead to mayhem.

[marvinpfoertner]

I discovered that redefined-variable-type is a message coming from the optional pylint plugin of the same name (in fact it is the plugin's only purpose to add this message). I will just remove the plugin from the list of loaded plugins. Hence, this is not a real pylint ignore anymore.

[pnkraemer]

I agree with your suggested exceptions. And looking at the current global exceptions (whoah, what a list!) I cannot come up with any more.

If I remember correctly from the last software meeting, the plan was to move the to-be-removed global exceptions to the tox.ini (and make them per-module exceptions) which are then removed module-per-module. I wouldn't be surprised if many of these global ignores are already superfluous.

[marvinpfoertner]

@pnkraemer Let's maybe discuss your proposal #523 (comment) here for documentation pruposes:

How do people feel about raising the "too-many-arguments" threshold from 5 to a larger number?

I find 5 a bit tedious to comply with everywhere. For example, all the top-level interface functions in probnum seem to have more than 5. Many of the lower-level methods also depend on a number of parameters. Usually, most of them are optional anyway. It also feels like defeating the purpose to put "pylint: disable="too-many-arguments" everywhere.

[marvinpfoertner]

@pnkraemer I agree, what threshold would you suggest?

[marvinpfoertner]

Or should we get rid of the message alltogether?

[pnkraemer]

Not sure. 10? This way, there would still be some exceptions (I think probsolve_ivp has more than 10), but I think it is okay that one has to flag those extreme outliers specifically. This probably should come with an increase of the too-many-locals threshold, too, because arguments count as local variables, if I remember correctly.

[marvinpfoertner]

Let's try 10 for now. Any objections? @JonathanWenger @mmahsereci

[JonathanWenger]

Sounds good to me!

[marvinpfoertner]

@pnkraemer Let's maybe discuss your proposal #523 (comment) here for documentation pruposes:

And, on a similar note, how about disabling "missing-function-docstring" for the tests? Most tests are one-liner-functions with (usually) a rather descriptive name. How much value does a (forced) docstring add there?

[marvinpfoertner]

@pnkraemer I'm all for it :)

[pnkraemer]

How about missing-class-docstring? (I think module docstrings remain useful in any case)

[marvinpfoertner]

Yup, that one should probably go as well 👍🏻

[marvinpfoertner]

Objections? @JonathanWenger @mmahsereci

[mmahsereci]

Sounds good to me 👍

[marvinpfoertner]

@mmahsereci and I noticed that the combination of Python type hints and the numpydoc style result in a couple of problems. Namely, it is mandatory to supply parameter, return and yield types in the numpydoc style. However, from a code quality and maintainability perspective, it does not make sense to state the typing info twice, especially since this is pretty much guaranteed to result in inconsistent information over time.

We agreed that the correct (that is to say backward compatible w.r.t. numpydoc) way to handle type hints is to format docstrings as follows:

Parameters
----------
param1
    Description of `param1`
param2
    Description of `param2`

Yields
-------
yield_name :
    Description of `yield_name`.

Returns
-------
return_name :
    Description of `return_name`.

i.e. simply omitting the type of parameters, returns, and yields, but still keeping the colon in returns and yield (though not for parameters). This way, we can distiguish between {return,yield}_name being the name of the return/yield or it being its type (see numpydoc for more details).

However, since this is not covered by the numpydoc specification, we obtain pylint errors and broken docs.
@mmahsereci and I suggest to ignore missing-return-doc for the time being, until we can get numpydoc, pylint and sphinx-napoleon to fix this.

[marvinpfoertner]

@JonathanWenger @mmahsereci @pnkraemer @schmidtjonathan If you have any objections, I'd be glad to discuss :)

[marvinpfoertner]

I added a numpydoc issue about this numpy/numpydoc#356

[marvinpfoertner]

I also opened a Sphinx issue to ensure correct rendering in the documentation sphinx-doc/sphinx#10134