Skip to content

Fix use of seasonal periodicity in naive model with mean strategy#917

Closed
Flix6x wants to merge 4 commits intosktime:mainfrom
SeitaBV:fix-GH907
Closed

Fix use of seasonal periodicity in naive model with mean strategy#917
Flix6x wants to merge 4 commits intosktime:mainfrom
SeitaBV:fix-GH907

Conversation

@Flix6x
Copy link
Copy Markdown
Contributor

@Flix6x Flix6x commented Jun 1, 2021

Reference Issues/PRs

Fixes #907

What does this implement/fix? Explain your changes.

Prepend rather than append NaN values in case of seasonal periodicity not fitting an integer number of times in the window length.

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

No new dependencies.

What should a reviewer concentrate their feedback on?

The added test, and how it fits in with other tests.

Any other comments?

No.

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.
For new estimators
  • I've added the estimator to the online documentation.
  • I've updated the existing example notebooks or provided a new one to showcase how my estimator works.

@Flix6x Flix6x requested a review from mloning as a code owner June 1, 2021 08:41
@mloning
Copy link
Copy Markdown
Contributor

mloning commented Jun 2, 2021

With the recently introduced docstring tests, you need to fix all docstrings the files you touched. You can use pydocstyle <file> to run the checks locally. Let me know if you need help with any of that!

@Flix6x
Copy link
Copy Markdown
Contributor Author

Flix6x commented Jun 7, 2021

I merged your main branch into this branch, which fixes the docstrings. Is this what you intended?

@mloning
Copy link
Copy Markdown
Contributor

mloning commented Jun 23, 2021

@fkiraly could I ask you to review this one? I think it's fine but would be good to have someone else look over the unit test.

Copy link
Copy Markdown
Contributor

@mloning mloning left a comment

Choose a reason for hiding this comment

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

Looks all good to me. Sorry for the slow review @Flix6x! - there's just a merge conflict now. Let me know if you want to resolve that, otherwise I can look into this.

@mloning
Copy link
Copy Markdown
Contributor

mloning commented Jul 9, 2021

Closed in favour of #1124

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Seasonal periodicity seems to work incorrectly (at least for naive forecasts with mean strategy)

2 participants