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 Inverse Gamma distribution #415

Merged
merged 6 commits into from
Jun 30, 2024
Merged

Conversation

meraldoantonio
Copy link
Contributor

@meraldoantonio meraldoantonio commented Jun 28, 2024

Reference Issues/PRs

Related to the development of BayesianLinearRegression class. (#358)

What does this implement/fix? Explain your changes.

This pull request implements the Inverse Gamma distribution and Beta Distribution as a new distribution class.

The Inverse Gamma implementation wraps around scipy.stats.invgamma to provide functionalities consistent with other distributions in the library. I introduced this distribution as the conjugate prior for the variance (σ²) of a Normal distribution for subsequent use in the BayesianLinearRegression class. (#358)

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

No

What should a reviewer concentrate their feedback on?

Correctness of implementation

Did you add any tests for the change?

No

Any other comments?

No

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.

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.

Yes, we´ll need these for the conjugates.

Can you kindly also add these to the API reference?
docs/source/api_reference

@fkiraly
Copy link
Collaborator

fkiraly commented Jun 28, 2024

also, you will need to update your fork from main and resolve any merge conflicts.

@meraldoantonio
Copy link
Contributor Author

Sure! I've added the two distributions to the API reference and resolved the merged conflicts

@meraldoantonio
Copy link
Contributor Author

meraldoantonio commented Jun 29, 2024

I noticed a bug in the plot method of the Binomial.
Screenshot 2024-06-29 at 12 41 32 PM
When I tried to plot the pmf, the resulting graph looks incorrect... (but the underlying pmf method shows the correct values). I will look into this..

@fkiraly
Copy link
Collaborator

fkiraly commented Jun 29, 2024

When I tried to plot the pmf, the resulting graph looks incorrect... (but the underlying pmf method shows the correct values). I will look into this.

Yes, this is expected given the current implementation but also a bug. Could you kindly open an issue and link your example and add my comments below?

Currently, the plotting function sampes a grid of x points, plots x, f(x) for the function and connects them. If the grid does not hit the mass points, the plot will look like this.

The only way to have the correct plot always is to know where the support is. That is currenty not inspectable from the object, and that's why I wanted the sets symbolism from #244..

Speaking of which, do you have an ETA for the next iteration, or are you no longer working on this, @VascoSch92? Any response is fine, just for planning.

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.

import in docstring example fails due to wrong module name

@meraldoantonio
Copy link
Contributor Author

Sorry about that - have fixed the docstring typo

@meraldoantonio
Copy link
Contributor Author

Also, have opened a new issue about the plot pmf bug #416

@fkiraly fkiraly added enhancement module:probability&simulation probability distributions and simulators implementing algorithms Implementing algorithms, estimators, objects native to skpro interfacing algorithms Interfacing existing algorithms/estimators from third party packages labels Jun 30, 2024
@fkiraly fkiraly merged commit b41ae36 into sktime:main Jun 30, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement implementing algorithms Implementing algorithms, estimators, objects native to skpro interfacing algorithms Interfacing existing algorithms/estimators from third party packages module:probability&simulation probability distributions and simulators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants