probabilistic-numerics / probnum

Probabilistic Numerics in Python.
http://probnum.org
MIT License
434 stars 56 forks source link

Refactor `probnum.filtsmooth` to resolve the pylint messages #157

Closed nathanaelbosch closed 3 years ago

nathanaelbosch commented 4 years ago

At the time of writing, probnum.filtsmooth triggers the following pylint messages:

Goal: Our long-term goal is to enable all of these checks for all parts of the project. Therefore, we should continuously work on fixing these. Currently, pylint is set up to have different checks for different modules, such that we can work on these problems individually and progress more quickly :)

To work on this issue you can check for these messages individually by calling:

pylint src/probnum/filtsmooth --disable=all --enable=<message>

Then, after fixing all errors/warnings, you should remove the exception from the corresponding line in ./tox.ini, such that this message will be tracked in the future.

pnkraemer commented 4 years ago

Thanks for that detailled summary! Some of those I have fixed already, some others are much harder to fix or perhaps should be ignored. I will make a WIP PR and give details below.

pnkraemer commented 4 years ago

How would you deal with the unused-argument for abstract base classes. My idea would be to disable this warning in each respective line, i.e.

    @abstractmethod
    def update(self, time, randvar, data, **kwargs):
        """
        Update step of the Bayesian filter.

        Must be implemented by subclasses.
        """
        raise NotImplementedError

becomes

    @abstractmethod
    def update(
            self,
            time,  # pylint: disable=unused-argument
            randvar,  # pylint: disable=unused-argument
            data,  # pylint: disable=unused-argument
            **kwargs  # pylint: disable=unused-argument
    ):
        """
        Update step of the Bayesian filter.

        Must be implemented by subclasses.
        """
        raise NotImplementedError

This is tedious, and potentially requires lots of C&P of # pylint: disable=unused-argument, but disabling this warning in principle seems too sledgehammerish to me.

nathanaelbosch commented 4 years ago

How would you deal with the unused-argument for abstract base classes.

The issue I see here is rather that BayesFiltSmooth.update is currently not an abstract method, since if it had the @abstractmethod decorator it would not trigger the "unused-argument" warning.

pnkraemer commented 4 years ago

Fair enough, but how would one deal with functions that raise a NotImplementedError without being an abstract method? For example the update. It is not an abstract method (particle filters do not have an update and should not be forced to implement it IMO) but having the interface seems useful to me.

nathanaelbosch commented 4 years ago

I also don't really know. I guess I'm not completely sure about the actual benefit of having this "interface", if not all subclasses actually use it, but I'm also fine with the way it is currently.

To answer your original question, if we want to keep the methods in the current state, the following should also work and would be less verbose:

    def update(self, time, randvar, data, **kwargs):
        """
        Update step of the Bayesian filter.

        Must be implemented by subclasses.
        """
        # pylint: disable=unused-argument
        raise NotImplementedError