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] Bayesian Linear Regression using Normal Conjugate Prior #500

Open
wants to merge 36 commits into
base: main
Choose a base branch
from

Conversation

meraldoantonio
Copy link
Contributor

What does this implement/fix? Explain your changes.

Implementing a new Bayesian linear regression estimator module using normal conjugate prior
with known noise variance.

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

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

@meraldoantonio meraldoantonio marked this pull request as draft November 22, 2024 01:42
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@@ -195,3 +195,14 @@ Base classes
:template: class.rst

BaseProbaRegressor

Bayesian
------------
Copy link
Collaborator

Choose a reason for hiding this comment

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

underline should be as long as the header. I would also put it in a single section with the other regressor.

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.

Great!

Especially kudos for getting the update right!
I have reviewed the math as well, and it looks correct!

Some comments:

  • changes to notebook 3 should be avoided - that is not part of this PR
  • is it not possible to set different priors for different coefficients? Might be nice to have, since these tend to be on different scales in real applications.

@meraldoantonio
Copy link
Contributor Author

Hi @fkiraly,

Thanks for the review! I’ve restructured the folders to improve organization. Now, we have a bayesian subfolder containing both of our new estimators. Similarly, the example notebooks have been moved into a bayesian subfolder inside the examples directory.

For this conjugate estimator, I haven’t yet enabled the setting of mu priors for different coefficients, as implementing this will require a more complex update formula. This will take a bit more time to complete.

Looking ahead, I suggest we create at least two versions of the conjugate estimator:

  1. Simple Version (the current one): Assumes the prior mu is all zeros, and the covariance matrix is isotropic.
  2. Flexible Version: Allows users to set both the priors mu and the covariance matrix covar.
  3. A longer term goal will be to create a Bayesian Linear Regressor for which both precision and mu are unknown; this will require us to use Normal-Wishart conjugate model. However, this probability distribution is not yet available in scipy (it's coming in the next version).

My next goal is to work on these later versions. Once it’s ready, we could rename the current estimator to something like SimpleBayesianConjugateLinearRegressor to distinguish it more clearly.

@meraldoantonio meraldoantonio marked this pull request as ready for review December 6, 2024 08:21
Bayesian
--------

.. currentmodule:: skpro.regression.bayesian.bayesian_conjugate
Copy link
Collaborator

Choose a reason for hiding this comment

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

the BayesianLinearRegressor is in another folder, so will not show up here.

It assumes a known noise precision beta for the Gaussian likelihood.
For inference, it returns a multivariate Normal distribution as the posterior.
For prediction, it returns a series of univariate Normals for each data point.

Copy link
Collaborator

Choose a reason for hiding this comment

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

do you want to add math to make this precise? Also, the handling of the intercept should be discussed.

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.

Great, only some smaller comments:

  • the BayesianLinearRegressor is not in the folder linked to in the API ref
  • non-blocking suggestions about improving the docstring

Non-blocking, to think about:

The notebook could be about Bayesian linear regression, not just about the conjugate approach. It would be informative to separately showcase the Bayes rule, and different ways to estimate/approximate the actual posterior. Conjucate and MCMC are two of a longer list (other strategies are variational and approximate).

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.

2 participants