Skip to content

Refactor reducer#1031

Merged
fkiraly merged 5 commits intosktime:mainfrom
Lovkush-A:refactor-reducer
Jun 24, 2021
Merged

Refactor reducer#1031
fkiraly merged 5 commits intosktime:mainfrom
Lovkush-A:refactor-reducer

Conversation

@Lovkush-A
Copy link
Collaborator

@Lovkush-A Lovkush-A commented Jun 23, 2021 •
edited
Loading

Reference Issues/PRs

#955

  • removed forecasting mixins and replaced with tags.
  • moved fit in _Reducer. there was one check in this fit that was moved to all the other _fit methods
  • added fh as parameter to all the _fit methods and adjusted docstring as such

Note that locally I checked only the tests for reduce, and those all passed. But who knows if other tests will fail... :/

@Lovkush-A Lovkush-A requested a review from mloning as a code owner June 23, 2021 15:38
@Lovkush-A
Copy link
Collaborator Author

Lovkush-A commented Jun 23, 2021

@fkiraly Please review this

mloning
mloning approved these changes Jun 23, 2021
View reviewed changes
Copy link
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.

@Lovkush-A Looks all good to me! Happy to merge this as it is.

Just a comment: Having a _fit method in the _Reducer class which implements the check_window_length call and then delegates to another _internal_fit(or whatever you want to call it) would be an alternative but seems a bit overcomplicated.

@Lovkush-A
Copy link
Collaborator Author

Lovkush-A commented Jun 23, 2021

Just a comment: Having a _fit method in the _Reducer class which implements the check_window_length call and then delegates to another _internal_fit(or whatever you want to call it) would be an alternative but seems a bit overcomplicated.

I had similar thoughts, but also think it would have been overcomplicated

@fkiraly fkiraly self-requested a review June 23, 2021 21:28
fkiraly
fkiraly approved these changes Jun 23, 2021
View reviewed changes
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.

happy too, let's just wait for tests to pass

@fkiraly
Copy link
Collaborator

fkiraly commented Jun 24, 2021

tests passed overnight - tests have re-started due to a PR that only fixed a broken link, so force-merging

@fkiraly fkiraly merged commit f3d3bc0 into sktime:main Jun 24, 2021
@fkiraly fkiraly mentioned this pull request Jun 24, 2021
Closed
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fkiraly fkiraly fkiraly approved these changes

+1 more reviewer

@mloning mloning mloning approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Lovkush-A @fkiraly @mloning