-
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] Creating a new Bayesian Regressor with PyMC as a backend #358
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
…ons have diff. behaviors wrt tensor mutability)
skpro/regression/bayesian.py
Outdated
# Priors for unknown model parameters | ||
self.intercept = pm.Normal("intercept", mu=self.intercept_mu, sigma=self.intercept_sigma) | ||
self.slopes = pm.Normal("slopes", mu=self.slopes_mu, sigma=self.slopes_sigma, shape = self._X.shape[1], dims=("pred_id")) | ||
self.noise = pm.HalfNormal("noise", sigma=self.noise_sigma) |
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.
would inverse gamma not be more standard here, as it is conjugate to the normal?
Yes it is!
Sure, noted! Yes, you're right, we can add the Also, I've documented the logic behind this class in this PR, could you please take a look at this when you have time? Thank you! |
Then you should adjust the tagging/signposting:
|
PS: there is a conflict in the |
PS on the
Instead of introducing a potentially misleading |
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 code looks great!
There are some testing issues:
- the failure on 3.9 still seems there, even though we clearly set the tag. Could you check if you can understand what is going on there?
get_test_params
should be populated with at least two examples.
I'm not sure what's going on with the failing tests with Python 3.9, I've tried playing around with the versions but somehow the test suite ignores this tag :( I've added examples to |
Summarizing our collaborative coding session on November 8, we have found out what the problem is in the failures on python 3.9, namely a known issue with the dependency checking framework that is already fixed in The todo is summarized in #489, and should happen in a separate pull request. |
oops, looks like this got accidentally closed, I reopened. The reason for this is, you accidentally used a reserved "closing keyword" in #490, that is, you wrote "fixes #358" in the preamble. If you do this, then the mentioned issue or PR gets closed once the PR is merged! I did not know btw that this also applied to PR, not just to issues. |
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.
Looks good now. I fixed a merge conflict in pyproject
, somehow the dead deps tabulate
and uncertainties
returned.
Reference Issues/PRs
#7
What does this implement/fix? Explain your changes.
This WIP PR implements a Bayesian Linear Regressor with PyMC as a backend
Does your contribution introduce a new dependency? If yes, which one?
Yes - it depends on PyMC family: PyMC itself, XArray and ArviZ
What should a reviewer concentrate their feedback on?
The design of the BayesianLinearRegressor. Especially:
Did you add any tests for the change?
Not yet
Any other comments?
N/A
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
(This is not yet done)
docs/source/api_reference/taskname.rst
, follow the pattern.Examples
section.python_dependencies
tag and ensureddependency isolation, see the estimator dependencies guide.