[ENH] @lru_cache in ForecastingHorizon is an error for flake8-bugbear

[khrapovs]

Is your feature request related to a problem? Please describe.

Starting from version 22.3.20 of flake8-bugbear (https://github.com/PyCQA/flake8-bugbear/releases/tag/22.3.20) @lru_cache decorator in sktime.forecasting.base._fh.ForecastingHorizon.to_relative and sktime.forecasting.base._fh.ForecastingHorizon.to_absolute raises a pre-commit error. Adding # noqa: B019 silences the warning, but I do not think this is a very sustainable solution.

Describe the solution you'd like

Is @lru_cache strictly necessary? Is the speed gain so dramatic that it overweights the dangers suggested by flake8? I do not have much experience with caching, so I have no good solution to propose.

[fkiraly]

@khrapovs, why does it raise the error?
I thought the B019 specification does this for class methods, which makes sense to me since since all objects share it, but this is an object method (not a class method), so it doesn't should like it should be raised?

[khrapovs]

No idea. I just tested the same pre-commit with 22.1.11 and with latest 22.3.23. In the later case pre-commit prevents a commit in _fh.py. In the former, commit works without warnings/errors.

[fkiraly]

Is that a bug in flake8-bugbear then? Since implemented functionality does not seem to comply with the specified functionality.

[fkiraly]

Well, it's called bugbear, so might be plausible?

[khrapovs]

I have asked the question about class methods in PyCQA/flake8-bugbear#218. Let's see what they answer.

[fkiraly]

maybe better in an open issue, since closed PR are less well tracked?

[khrapovs]

Opened the issue: PyCQA/flake8-bugbear#250

[khrapovs]

So, the question was answered. There is no error, no bug. It is only our (or rather my) lack of understanding how caching works in combination with classes and their methods. I see two ways to proceed:

1. At least temporary add # noqa: B019 comment next to @lru_cache. This would allow to make changes in sktime.forecasting.base._fh module.
2. Address a deeper issue with caching. Again, is it really necessary? Personally, I can not say.

[fkiraly]

The "simple" solution would be to move this to loose functions to_absolute(fh, cutoff) and to_relative(fh, cutoff) which are then referenced from within the class, no?

[khrapovs]

@fkiraly Something like that #2364?