-
Notifications
You must be signed in to change notification settings - Fork 51
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
Conversation
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).
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.
Excellent work! My comments are mainly on documentation, PR structure, nothing content-wise.
- the notebooks should be reconciled with PR [DOC] Adding a Notebook companion to BayesianMCMCLinearRegressor, Restructuring Estimator Folder #480, otherwise I see some inconsistencies that could cause merge headaches. I would suggest to split off the notebooks in a separate PR, with dependency graph either 500, 480 -> new PR, or 500 -> 480 -> new PR (left side of arrow gets merged earlier).
- in the docstring, I think it is essential to say what exactly the model assumptions are, a little bit of math would be great there, and how these map onto python parameters.
- the changes in [DOC] Adding a Notebook companion to BayesianMCMCLinearRegressor, Restructuring Estimator Folder #480 are inconsistent with the directory structure here - I would recommend to make all submodues of
bayesian
private, as long as we are not certain about the exact end state desired.
Hi @fkiraly, Thanks for the review! I've made the following updates:
Let me know if this works! |
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.
Works for me, all perfect.
Maybe call the new API section not "Bayesian" but "Bayesian regressors" or similar? "Bayesian" is just an adjective (and/or the name of a sunken yacht).
Hi @fkiraly, I've modified the API name in |
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).