-
Notifications
You must be signed in to change notification settings - Fork 45
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] (WIP) Creating a new Bayesian Regressor with PyMC as a backend #358
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 |
…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?
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.
Nice contribution!
Some high-level points:
- could you split the notebook off into a chained PR, based on the estimator PR? The notebook may require some more review (time), and should not block the estimator
- in the estimator, I would kindly ask you to remove the visualization dependencies from
python_dependencies
, and instead introduce dependency checks in the methods that need them? This way, users do not need to visualisation dependencies when using the model in a deployment pipeline.
Strange import error - is this related to an upper bound of any of the imports implied by 3.9, e.g., |
Apparently there is a bug with Arviz 0.17 and scipy>=1.13 (source 1) (source 2). The bug is no longer present in Arviz 0.18 but this requires Python 3.10 and above. As a temporary solution, I've locked the scipy version in |
Makes sense. From a maintenance perspective, applying the version bound in the Could you add the lock instead in the |
…ressor dependencies
Makes sense! But I've tried this a couple of times and for some reason, without the It might be that other libraries are pulling in a conflicting version, but I haven't managed to find the exact cause.. Any ideas? |
Why are you trying to bound |
PS: why did you close the notebook PR? That was a nice notebook, and indeed it would be nice as separate PR. |
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.