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

[MNT] carry out accidentally missed deprecation action for 0.15.0: in WEASEL and BOSS, remove type_dict and update default alphabet_size=2 #4025

Merged
merged 8 commits into from
Dec 31, 2022

Conversation

xxl4tomxu98
Copy link
Contributor

@xxl4tomxu98 xxl4tomxu98 commented Dec 30, 2022

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Carries out missed deprecation actions for WEASEL and BOSS for 0.15.0 (missed since TODO note did not include full version number):

  • remove type_dict argument
  • change default alphabet_size parameter value to 2

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], [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
Copy link
Collaborator

fkiraly commented Dec 30, 2022

No more notebooks, great!

Can you kindly copy over the pull request description from your first PR and close the other one still open?

@fkiraly
Copy link
Collaborator

fkiraly commented Dec 30, 2022

also, kindly feel free to add yourself to the allcontributorsrc file for contributions!

@fkiraly fkiraly added the maintenance Continuous integration, unit testing & package distribution label Dec 30, 2022
@fkiraly fkiraly changed the title [MNT] remove type_dict and update variable alphabet_size to 2 [MNT] carry out accidentaly missed deprecation action for 0.15.0: in WEASEL and BOSS, remove type_dict and update default alphabet_size=2 Dec 30, 2022
@fkiraly fkiraly changed the title [MNT] carry out accidentaly missed deprecation action for 0.15.0: in WEASEL and BOSS, remove type_dict and update default alphabet_size=2 [MNT] carry out accidentally missed deprecation action for 0.15.0: in WEASEL and BOSS, remove type_dict and update default alphabet_size=2 Dec 30, 2022
@xxl4tomxu98
Copy link
Contributor Author

added PR description and pushed up updates in allcontributorsrc

fkiraly
fkiraly previously approved these changes Dec 30, 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 to me now, thanks for helping out with the missed deprecation!

@xxl4tomxu98
Copy link
Contributor Author

@fkiraly, I look at the failed tests, both test_windows(3.8) and test_unix(3.10) failed on the same module, FAILED sktime/classification/tests/test_all_classifiers.py::TestAllClassifiers::test_classifier_on_unit_test_data[BOSSEnsemble] - AssertionError: Arrays are not almost equal to 2 decimals I could not locate the file from the folder specified.

@fkiraly
Copy link
Collaborator

fkiraly commented Dec 30, 2022

The reason is a unit test which checks predictions on a dummy data set against hard-coded expectations.

Since the default parameters have changed, the predictions will also change.
I've updated the parameter_set in get_test_params for results_comparison to match the old parameters, this should fix it.

@fkiraly
Copy link
Collaborator

fkiraly commented Dec 31, 2022

One more thing, carrying out the deprecation action does not make you an owner of the estimator. I've removed you from the owners list.

To clarify the rules: you become an owner only via three conditions:

  • you contribute the estimator (by default contributor is owner)
  • existing owners add you as an owner
  • existing owners become unreachable, and sktime gov bodies decide to appoint you as an owner

Nb: if you want to make a change that the original owners don't like, you can always make a copy and have your own version of the estimator (as long as you give proper scientific credit to the original code, you can own the copy).

This PR was maintenance - it's carrying out an intended action by the owner that the release manager (for 0.15.0, that was me) should have carried out.

I.e., you were fixing my mistake in carrying out what the owner wanted (which had to be delayed to a MINOR release due to sktime's deprecation policy) - let me know if you have any questions.

Thanks for helping out!

@xxl4tomxu98
Copy link
Contributor Author

OK @fkiraly , understand! As I am intending to contribute more in the future, learning these rules is definitely helpful. Thanks

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 for your contribution!

@fkiraly fkiraly merged commit 236dfec into sktime:main Dec 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Continuous integration, unit testing & package distribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants