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

[BUG] Constructor of any DL estimator to pass non-default values to underlying Network object #4075

Merged
merged 3 commits into from Jan 11, 2023
Merged

[BUG] Constructor of any DL estimator to pass non-default values to underlying Network object #4075

merged 3 commits into from Jan 11, 2023

Conversation

achieveordie
Copy link
Collaborator

@achieveordie achieveordie commented Jan 6, 2023

Reference Issues/PRs

Fixes #4067

What does this implement/fix? Explain your changes.

  • Manually correct behaviour for all current estimators by passing all values from estimator object to network object during estimator initialization.
  • Add a test to ensure this works properly.

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

No

What should a reviewer concentrate their feedback on?

We need to discuss if there is another way that is superior to the current solution.

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

fkiraly
fkiraly previously approved these changes Jan 9, 2023
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 reasonable and like a good fix, but is still a draft - is this ready to be merged?

@achieveordie achieveordie marked this pull request as ready for review January 10, 2023 08:30
@achieveordie
Copy link
Collaborator Author

I've added a test to ensure it doesn't fail in the future. Should be ready to be merged now.

@fkiraly
Copy link
Collaborator

fkiraly commented Jan 10, 2023

seems to have crashed while downloading prophet? I've restarted the failed CI now, I think it's queued.

@fkiraly
Copy link
Collaborator

fkiraly commented Jan 10, 2023

failed again with timeout while downloading prophet from pypi? What is going on there?

@achieveordie
Copy link
Collaborator Author

I guess something was down temporarily, I saw the failures on other checks as well. I wonder why docs are not being built, any idea?

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.

Straightforward fix for a hard-to-detect and quite consequential bug.

Seems like all params are now passed on. We can deal with the general design problem later.

@fkiraly fkiraly merged commit 4f140f2 into sktime:main Jan 11, 2023
@achieveordie achieveordie deleted the dl_constructor branch January 12, 2023 09:55
klam-data pushed a commit to CodeSmithDSMLProjects/sktime that referenced this pull request Jan 18, 2023
…nderlying `Network` object (sktime#4075)

Fixes sktime#4067:

- [x] Manually correct behaviour for all current estimators by passing all values from estimator object to network object during estimator initialization.
- [x] Add a test to ensure this works properly.
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] Initializing any DL estimator with non-default values constructs these values only superficially
3 participants