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] added piecewise_multinomial function to datagen.py #4079

Merged
merged 3 commits into from
Jan 16, 2023

Conversation

JonathanBechtel
Copy link
Contributor

Reference Issues/PRs

PR #3363

What does this implement/fix? Explain your changes.

In order to complete the unit tests for importing different modules in HMMLearn, additional data generation functions needed to be written for the appropriate modules. This pull requests added a function called piecewise_multinomial that generates multinomial data to be used in unit tests.

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

No

What should a reviewer concentrate their feedback on?

Making sure the random number generation works in a way that's compatible with the way HMMLearn will ingest it.

PR checklist

For all contributions
  • [ X] The PR title starts with either [ENH], [MNT], [DOC], or [BUG] indicating whether the PR topic is related to enhancement, maintenance, documentation, or bug.

miraep8
miraep8 previously approved these changes Jan 7, 2023
Copy link
Collaborator

@miraep8 miraep8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just small non-blocking comment. However - may I also request that if you haven't already added yourself as a contributor to sktime that you do so now? :) Cheers!

sktime/annotation/datagen.py Show resolved Hide resolved
sktime/annotation/datagen.py Outdated Show resolved Hide resolved
sktime/annotation/datagen.py Outdated Show resolved Hide resolved
@lmmentel
Copy link
Contributor

lmmentel commented Jan 7, 2023

Thanks @JonathanBechtel 🙌 Overall looks good. I left a few comments that I hope can make the code a bit more robust.

@JonathanBechtel
Copy link
Contributor Author

@lmmentel @miraep8 Thank you, will update accordingly.

@miraep8
Copy link
Collaborator

miraep8 commented Jan 13, 2023

This should be merge-able once those changes are propagated! :) Happy to take another look then!

@JonathanBechtel
Copy link
Contributor Author

JonathanBechtel commented Jan 14, 2023

@lmmentel @miraep8 Made the suggested changes, thank you for the tips. To recap:

  • updated docstrings to include more detail about output of function
  • removed assert statements and changed them to if statements w/ a raise statement as suggested
  • changed pvals to p_vals to make arguments more clear

Thanks again for looking at it.

@JonathanBechtel JonathanBechtel requested review from lmmentel and miraep8 and removed request for fkiraly, lmmentel and miraep8 January 16, 2023 00:11
Copy link
Contributor

@lmmentel lmmentel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Collaborator

@miraep8 miraep8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me as well! Thanks for addressing all the change requests! :) 🎉

@miraep8 miraep8 merged commit 5b1635e into sktime:main Jan 16, 2023
klam-data pushed a commit to CodeSmithDSMLProjects/sktime that referenced this pull request Jan 18, 2023
Adding piecewise multinomial data generating function.
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.

None yet

3 participants