-
Notifications
You must be signed in to change notification settings - Fork 11
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
Added the ability to use safe priors for hierarchical models #331
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.
I think this looks good now.
Looked over it again and taking into account our discussion from yesterday (thurs)
@@ -24,6 +24,7 @@ | |||
import seaborn as sns | |||
import xarray as xr | |||
from bambi.model_components import DistributionalComponent | |||
from bambi.transformations import transformations_namespace |
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 is that one for actually?
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 think there is a default namespace that has to be included. This can be found in Bambi source code here
@@ -531,3 +654,14 @@ def _make_default_prior(bounds: tuple[float, float]) -> bmb.Prior: | |||
return bmb.Prior("TruncatedNormal", mu=lower, lower=lower, sigma=2.0) | |||
else: | |||
return bmb.Prior(name="Uniform", lower=lower, upper=upper) | |||
|
|||
|
|||
def merge_dicts(dict1: dict, dict2: dict) -> dict: |
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.
strictly speaking this is not a merge right?
seems like if key in merged
but instances are not dicts, you override the value of dict1 with whatever you find in dict2.
So maybe something like override_dict
?
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 actually raises a question: should the override be recursive? For example, if 1|participant_id
has a defined prior, with only sigma
having a defined hyperprior, should we also add our default mu
? It seems that if we don't then a bambi
or pymc
default will be applied
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 would say no, because we would in fact not cover the case in all generality even then.
The logic would only apply if the user defined distribution 1|participant_id
matches the one for which we have hyper-priors available.
If the user defines their own prior, they should define it all the way down I would say.
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 :).
couldn't find any serious issue anymore.
pyproject.toml
Outdated
@@ -23,14 +23,14 @@ numpy = ">=1.23.4,<1.26" | |||
onnx = "^1.12.0" | |||
jax = "^0.4.0" | |||
jaxlib = "^0.4.0" | |||
ssm-simulators = "0.5.1" | |||
ssm-simulators = "0.5.3" |
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.
can be bumped to 0.6.1 now
No description provided.