-
Notifications
You must be signed in to change notification settings - Fork 81
Support for custom priors via Prior class #488
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
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.
This is very cool!
As far as I can tell, these changes are currently entirely invisible to the end user, right?
Before we do a new release I will follow up with another PR that adds some documentation. This will most likely be in the form of just giving a couple of worked examples
Main request is to add custom-ness to the Dirichlet for the WeightedSumFitter
class. Like I say in a comment, I mostly see people wanting to customise the hyperparams, not the distribution itself.
Inference data... | ||
""" # noqa: W605 | ||
|
||
default_priors = { |
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.
need to add @property
decorator here? Or is that remembered from it being done in the PyMCModel
base class?
Getting an Pylance warning: Type "dict[str, Prior]" is not assignable to declared type "property"
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.
What line of code bring that on? Maybe having a setter will help?
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.
Agree, if you want a property, maybe we can have a setter method? (not a blocker for now and maybe create an issue?)
causalpy/pymc_models.py
Outdated
n_predictors = X.shape[1] | ||
X = pm.Data("X", X, dims=["obs_ind", "coeffs"]) | ||
y = pm.Data("y", y[:, 0], dims="obs_ind") | ||
beta = pm.Dirichlet("beta", a=np.ones(n_predictors), dims="coeffs") |
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.
We definitely want a custom prior for the Dirichlet. I think the Dirichlet would be used always (or nearly always), but there are plenty of real world use cases where the user might want to change the hyper parameters (currently a=np.ones(n_predictors)
).
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.
Alright. since it is function of data, we will have to handle differently
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.
I've address this with dc20e3e. If user wants to change, they can add to the "priors" at initialization.
Take a look and let me know what you think.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #488 +/- ##
==========================================
+ Coverage 95.19% 95.43% +0.23%
==========================================
Files 28 28
Lines 2457 2586 +129
==========================================
+ Hits 2339 2468 +129
Misses 118 118 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tests pass :) |
Excited about this PR! |
causalpy/pymc_models.py
Outdated
n_predictors = X.shape[1] | ||
|
||
return { | ||
"beta": Prior("Dirichlet", a=np.ones(n_predictors), dims="coeffs"), |
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.
Just realised that n_predictors
will equal length of the "coeffs" dim. Does that have to be the case in fact? If so, do we need priors_from_data
?
Not saying I don't want it, it could be really cool. But just wondering more generally if it's needed or not. Will try to think more with a fresh head in the morning, but does this spark off any thoughts?
So I spent some time doing a gnarly merge from main into this dev branch. There were some widespread changes in main, so it took me a while to get all the tests passing. But it should make your life easier :) Remote doctests failing. They pass for me locally. Seems maybe like an import error in pymc? Any ideas @williambdean ? |
- Updated LinearRegression default_priors to support multi-unit architecture - Resolved print_coefficients method conflicts - Added patsy import back for dmatrix usage - Regenerated interrogate_badge.svg with updated coverage
- Use Prior class create_likelihood_variable method for model definitions - Updated print_coefficients to handle both sigma and y_hat_sigma variables - Simplified PropensityScore to use Prior class exclusively - Deleted interrogate badge to be regenerated
This seems to be the reason why the remote tests are failing: pymc-devs/pymc-extras#541 |
The issue is that pymc-extras 0.3.0+ requires Python >=3.11, but the CI is running on Python 3.10.18. That's why pip can't find a compatible version. |
Added '# pragma: no cover' to NotImplementedError and ValueError branches in PyMCModel to exclude them from test coverage reporting.
Remote tests back to passing :) |
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! There is a comment by @williambdean which we could tackle later (please create an issue :) )
Inference data... | ||
""" # noqa: W605 | ||
|
||
default_priors = { |
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.
Agree, if you want a property, maybe we can have a setter method? (not a blocker for now and maybe create an issue?)
Thanks for the review @juanitorduz. My feeling at the moment is... The current workflow is that computation is kicked off automatically when creating an experiment object instance. As in, you don't first create the experiment then manually call a Changed |
Can deal with this (if at all) in another PR
This using
pymc-extras
to allow for custom priors. Might need to adjust the remaining a bit to work.Closes #387
@drbenvincent did some work to: