aeon-toolkit / aeon

A toolkit for machine learning from time series
https://aeon-toolkit.org/
BSD 3-Clause "New" or "Revised" License
992 stars 117 forks source link

[MNT] Ensure base classes are abstract and adjust testing #2177

Open CodeLionX opened 2 weeks ago

CodeLionX commented 2 weeks ago

Describe the issue

As discussed in https://github.com/aeon-toolkit/aeon/pull/2118#discussion_r1796538246 and Slack, our current testing framework checks that every estimator class has an implementation for _fit/_predict/_fit_predict using

>>> # Test that all anomaly detectors implement abstract predict.
>>> assert "_predict" in estimator_class.__dict__
E       AssertionError

This, however, just checks within the final class definition and ignores any potential implementations in the class hierarchy. Thus, in wrapper-land, we are required to overwrite these methods only to call the super-implementation, e.g.:

from aeon.anomaly_detection import PyODAdapter

class IsolationForest(PyODAdapter):
    def __init__(self, window_size, stride, **params):
        from pyod.models.iforest import IForest
        ...
        super().__init__(IForest(**params), window_size, stride)

    def _fit(self, X, y=None):
        super()._fit(X, y)

    def _predict(self, X):
        return super()._predict(X)

    def _fit_predict(self, X, y=None):
        return super()._fit_predict(X, y)

I would like to avoid the boilerplate code and reduce the wrapper to the class-definition, documentation, and constructor.

Suggest a potential alternative/fix

Python's abstract base classes already provide a way to enforce that abstract methods must be implemented. Otherwise, the instantiation of the class fails.

If we ensure that all our base classes inherit from abc.ABC and annotate their abstract methods with abc.abstractmethod, we can remove the testing for implementations of _fit/_predict/_fit_predict, and just test the instantiation of the estimator to not raise a TypeError: Can't instantiate abstract class XYZ with abstract method _xyz.

Additional context

No response

MatthewMiddlehurst commented 5 days ago

2171 adds ABC to BaseAeonEstimator for the first step of this.