-
Notifications
You must be signed in to change notification settings - Fork 47
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
base: main
Are you sure you want to change the base?
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
@@ -195,3 +195,14 @@ Base classes | |||
:template: class.rst | |||
|
|||
BaseProbaRegressor | |||
|
|||
Bayesian | |||
------------ |
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
Hi @fkiraly, Thanks for the review! I’ve restructured the folders to improve organization. Now, we have a For this conjugate estimator, I haven’t yet enabled the setting of Looking ahead, I suggest we create at least two versions of the conjugate estimator:
My next goal is to work on these later versions. Once it’s ready, we could rename the current estimator to something like |
Bayesian | ||
-------- | ||
|
||
.. currentmodule:: skpro.regression.bayesian.bayesian_conjugate |
There was a problem hiding this comment.
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. | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this 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).
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
How to: add yourself to the all-contributors file in the
skpro
root directory (not theCONTRIBUTORS.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 pluscode
if you also fixed the bug in the PR).maintenance
- CI, test framework, release.See here for full badge reference
For new estimators
docs/source/api_reference/taskname.rst
, follow the pattern.Examples
section.(https://www.sktime.net/en/latest/developer_guide/dependencies.html#adding-a-soft-dependency).