Skip to content

[ENH] Beta Distribution#298

Merged
fkiraly merged 4 commits into
sktime:mainfrom
malikrafsan:beta-distribution
May 7, 2024
Merged

[ENH] Beta Distribution#298
fkiraly merged 4 commits into
sktime:mainfrom
malikrafsan:beta-distribution

Conversation

@malikrafsan

@malikrafsan malikrafsan commented May 4, 2024

Copy link
Copy Markdown
Contributor

Reference Issues/PRs

Towards #22

What does this implement/fix? Explain your changes.

This PR implements Beta distribution based on Scipy Adapter

What should a reviewer concentrate their feedback on?

  • Whether the Scipy Adapter makes it easier to implement new distribution

Did you add any tests for the change?

Any other comments?

PR checklist

For all contributions
  • I've added myself to the list of contributors with any new badges I've earned :-)
    How to: add yourself to the all-contributors file in the skpro root directory (not the CONTRIBUTORS.md). Common badges: code - fixing a bug, or adding code logic. doc - writing or improving documentation or docstrings. bug - reporting or diagnosing a bug (get this plus code if you also fixed the bug in the PR).maintenance - CI, test framework, release.
    See here for full badge reference
  • The PR title starts with either [ENH], [MNT], [DOC], or [BUG]. [BUG] - bugfix, [MNT] - CI, test framework, [ENH] - adding or improving code, [DOC] - writing or improving documentation or docstrings.
For new estimators
  • I've added the estimator to the API reference - in docs/source/api_reference/taskname.rst, follow the pattern.
  • I've added one or more illustrative usage examples to the docstring, in a pydocstyle compliant Examples section.
  • If the estimator relies on a soft dependency, I've set the python_dependencies tag and ensured
    dependency isolation, see the estimator dependencies guide.

@malikrafsan malikrafsan marked this pull request as ready for review May 4, 2024 13:49
Comment thread docs/source/api_reference/distributions.rst Outdated
Comment thread skpro/distributions/__init__.py Outdated

class Beta(_ScipyAdapter):
r"""Beta distribution.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

could you add a short note about parameterization? Have a look at the other distributions.

The reason is, many distributions have different parameterizations, e.g., normal can be parameterized by sigma or sigma-squared. It suffices to spell out one of the distribution defining functions, usually I choose the "simplest" for users to understand.

Let me know if you are unsure, then I will add it later.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Recently, I have added the parameterization note about the beta distribution. I had a look at the other distributions, and a lot of them are noted by PDF or CDF formula. That's why I tried to add the PDF formula as I think it is easier to understand, in the context of Beta Distribution. I took the formula from the Wikipedia. I added the PDF formula as given

$$f(x) = \frac{x^{\alpha-1}(1-x)^{\beta-1}}{B(\alpha, \beta)}$$

Please let me know if there is something I missed! Also, I am totally fine as well if you want to modify the PR as well

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

yes, that works, especially with the explanation that B is for normalization.

@fkiraly fkiraly left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Excellent - it looks like we have minimized the effort to add more distributions with the adapter!

I suppose this takes 5 or 10 min per distribution now?

I left a few comments.

@fkiraly fkiraly added enhancement module:probability&simulation probability distributions and simulators labels May 4, 2024
@fkiraly

fkiraly commented May 4, 2024

Copy link
Copy Markdown
Collaborator

Whether the Scipy Adapter makes it easier to implement new distribution

Well, you tell me - you implemented it, was it easier?

@malikrafsan

Copy link
Copy Markdown
Contributor Author

Whether the Scipy Adapter makes it easier to implement new distribution

Well, you tell me - you implemented it, was it easier?

Sure! It was much easier compared to when I tried to implement the Weibull / LogLogistic distributions 😅

@malikrafsan malikrafsan requested a review from fkiraly May 6, 2024 23:23
@fkiraly fkiraly changed the title [ENH] Add Beta Distribution [ENH] Beta Distribution May 7, 2024

@fkiraly fkiraly left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

All addressed!

@fkiraly fkiraly merged commit 9de9f9c into sktime:main May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement module:probability&simulation probability distributions and simulators

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants