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] Wrapper for hmmlearn #3156

Closed
wants to merge 30 commits into from
Closed

[ENH] Wrapper for hmmlearn #3156

wants to merge 30 commits into from

Conversation

miraep8
Copy link
Collaborator

@miraep8 miraep8 commented Aug 1, 2022

Related to my issue #3099 on how I plan to further improve our support of HMM-like algorithms in sktime and also the issue I opened in hmmlearn (#484)

Will make a non-draft PR when it is ready for review. At a high level the plan is o have a simple base class that interfaces with the BaseAnnotator class and provides interface compatibility. Then each HMM-like class which inherits from this base class simply imports the relevant HMM estimator from hmmlearn.

Still to do:

  • make sure parameter setting is consistent/works!
  • add some more tests that things work as expected.

@miraep8 miraep8 marked this pull request as ready for review August 24, 2022 17:12
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.

All looks good, except the implicit change to the interface contract in "object".

Is there a way without changing the heart of sktime interface specs?

Worst case, lots of typing of self.=...
for a slightly less exhausting route, have a look at the ARIMA class.

@miraep8
Copy link
Collaborator Author

miraep8 commented Aug 26, 2022

Removed the attrs bit now and still seems to work ok

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, but your doctest is failing.

If you want expressions that continue over multiple lines, you need to use ... from the 2nd line on, instead of >>>

@KatieBuc
Copy link
Contributor

KatieBuc commented Sep 7, 2022

Looks great @miraep8!

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.

Code looks good to me!

May I ask for some changes re soft dependency handling, the general rule is "don't import soft dependencies directly, anywhere". So:

  • in the test, use a pytest.skipif and import hmmlearn inside the test. Compare how pmdarima is handled, search for pytest.skipif.
  • set the python_dependencies tag in the new classes (base class is enough)
  • skip the example doctest (search for # doctest to see examples). If this reduces coverage, move the examples into another test decorated with pytest.skipif.

@miraep8
Copy link
Collaborator Author

miraep8 commented Sep 21, 2022

Since GMMHMM has already been merged and that PR was a superset of this one I am going to close this PR.

@miraep8 miraep8 closed this Sep 21, 2022
@fkiraly
Copy link
Collaborator

fkiraly commented Sep 21, 2022

I see, I should have merged them in a sequence.

Sorry for this.

Next time, may I kindly ask to write sth explicit like "contains PR # 3156" in the other PR instead of sth flowery like "building on success in #3156" (which can simply mean you are following the same pattern).

@lmmentel lmmentel moved this from In progress to Done in Workstream: annotation, segmentation Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants