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] DummyRegressor for time series regression #3968

Merged
merged 10 commits into from Dec 22, 2022

Conversation

badrmarani
Copy link
Contributor

@badrmarani badrmarani commented Dec 20, 2022

Reference Issues/PRs

Fixes #3899.

What does this implement/fix? Explain your changes.

Adds a DummyRegressor that wraps over a sklearn DummyRegressor and inherits from BaseRegressor.

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

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.
  • The PR title starts with either [ENH], [MNT], [DOC], or [BUG] indicating whether the PR topic is related to enhancement, maintenance, documentation, or bug.

@fkiraly
Copy link
Collaborator

fkiraly commented Dec 20, 2022

Nice! I've started the CI.

@fkiraly fkiraly added the module:regression regression module: time series regression label Dec 20, 2022
@fkiraly
Copy link
Collaborator

fkiraly commented Dec 21, 2022

linting is failing, kindly ensure you format your code correctly.
Guideline here: https://www.sktime.org/en/stable/developer_guide/coding_standards.html

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.

(see above)

@badrmarani badrmarani requested review from fkiraly and removed request for aiwalter and GuzalBulatova December 21, 2022 04:28
@badrmarani
Copy link
Contributor Author

Hi,

I've gone ahead and fixed the linting issues on my last pull request. I believe everything should be good to go now.

Thank you!

@fkiraly
Copy link
Collaborator

fkiraly commented Dec 21, 2022

Tests are now genuinely failing. I think this may be one or both fo the following reasons:

  • regressors should return floats in predict, and expect float y in fit - not strings
  • when you compare equality with np.all, you might be missing a list comprehension

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.

thanks a lot, useful!

there are some small fiddly issues to sort out, see the failing tests

@badrmarani
Copy link
Contributor Author

I've gone ahead and fixed the unit test related errors. Code should now be able to pass the tests.

@fkiraly
Copy link
Collaborator

fkiraly commented Dec 22, 2022

Nice, they pass now! Thank you!

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.

Great!

I made a minor change: put the docs section in alphabetical order

@fkiraly fkiraly added the enhancement Adding new functionality label Dec 22, 2022
@fkiraly fkiraly changed the title [ENH] Add DummyRegressor [ENH] DummyRegressor for time series regression Dec 22, 2022
@fkiraly fkiraly merged commit 397f4ee into sktime:main Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adding new functionality module:regression regression module: time series regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENH] DummyRegressor for straw man comparison when benchmarking
2 participants