Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ENH] Extract cached ForecastingHorizon methods to functions and avoid B019 error #2364

Merged
merged 5 commits into from Apr 1, 2022
Merged

[ENH] Extract cached ForecastingHorizon methods to functions and avoid B019 error #2364

merged 5 commits into from Apr 1, 2022

Conversation

khrapovs
Copy link
Contributor

@khrapovs khrapovs commented Apr 1, 2022

Reference Issues/PRs

closes #2338

What does this implement/fix? Explain your changes.

Extract ForecastingHorizon.to_relative and ForecastingHorizon.to_absolute to _to_relative and _to_absolute, respectively, to allow caching without B019 error.

Does your contribution introduce a new dependency? If yes, which one?

No

What should a reviewer concentrate their feedback on?

Any other comments?

PR checklist

For all contributions
  • I've added myself to the list of contributors.
  • Optionally, I've updated sktime's CODEOWNERS to receive notifications about future changes to these files.
  • I've added unit tests and made sure they pass locally.
  • The PR title starts with either [ENH], [DOC] or [BUG] indicating wether the PR topic is related to enhancement, documentation or bug

fkiraly
fkiraly previously approved these changes Apr 1, 2022
Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

Non-blocking comments:

  • might be worth to add to the comments preceding the new, losse functions, a refrence to the B019, or someone might get the idea to put them back
  • you remember the pandas bug? I think we should maybe change the comments there from "replace when fixed" to "replace when we upgrade our lower pandas bound to a version where this is fixed"? Or we´ll have the bug back on the lower end of our supported versions...

@khrapovs
Copy link
Contributor Author

khrapovs commented Apr 1, 2022

@fkiraly Addressed both of your suggestions.

@fkiraly
Copy link
Collaborator

fkiraly commented Apr 1, 2022

@fkiraly Addressed both of your suggestions.

Thanks!

May I ask for reformulation of this:

This function is a direct replacement of the ForecastingHorizon.to_relative method to avoid B019 error from flake8-bugbear.

The problem with this is that it's obsccure.
You need to imagine someone who has not been part of our conversation.
Hence they will not get the reference "replacement" which is referring to the earlier state, since they will see only the current state (in the future).

Would suggest to change to

"This function needs to be outside ForecastingHorizon since the lru_cache decorator has known, problematic interactions with object methods, see B019 error of flake8-bugbear for a detail explanation."

@khrapovs
Copy link
Contributor Author

khrapovs commented Apr 1, 2022

@fkiraly I change the wording as you proposed

@fkiraly
Copy link
Collaborator

fkiraly commented Apr 1, 2022

readthedocs is fine (was before), but failing since too many readthedocs jobs are running concurrently

@fkiraly fkiraly merged commit 615e0c2 into sktime:main Apr 1, 2022
@khrapovs khrapovs deleted the 2338-khrapovs-to-absolute-to-relative-fiunctions branch April 2, 2022 08:16
@lmmentel lmmentel added the bugfix Fixes a known bug or removes unintended behavior label Apr 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes a known bug or removes unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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