-
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
Fix global formulat bug #604
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.
Looks good to me! Nit-picking in a few places. Approved once all checks pass
src/hssm/hssm.py
Outdated
@@ -24,6 +24,7 @@ | |||
import seaborn as sns | |||
import xarray as xr | |||
from bambi.model_components import DistributionalComponent | |||
from bambi.priors.prior import Prior |
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 it's more conventional to use from bambi import Prior
. Or, since we have imported bambi as bmb, we can just use bmb.Prior whenever you need it?
src/hssm/hssm.py
Outdated
self.formula, self.priors, self.link = self.params.parse_bambi(model=self) | ||
self.formula, priors, self.link = self.params.parse_bambi(model=self) |
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.
On second though, it might still be a good idea to have self.priors
, which makes it easy to see what was actually passed to bambi, if not only for debugging purposes. We might just want to make it clear in the documentation
src/hssm/hssm.py
Outdated
if isinstance(self.params[tmp_param].prior, dict): | ||
prior_tmp = getattr(self.params[tmp_param], "prior") |
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 looks slightly weird to me. You were already using self.params[tmp_param].prior
, effectively asserting that self.params[tmp_param]
has a prior
property, and then use getattr
to dynamically retrieve the value of that property.
Maybe we can use the walrus operator like this: if isinstance(prior_tmp := self.params[tmp_param].prior, dict):
, so we don't need the second line?
src/hssm/hssm.py
Outdated
if return_value: | ||
return args_tmp["initval"] if "initval" in args_tmp else None |
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.
if return_value: | |
return args_tmp["initval"] if "initval" in args_tmp else None | |
if return_value: | |
return args_tmp.get("initval", None) |
src/hssm/hssm.py
Outdated
return None | ||
else: | ||
return False | ||
return True if "initval" in args_tmp else False |
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.
return True if "initval" in args_tmp else False | |
return "initval" in args_tmp |
This fixes a bug that raised a
KeyError
upon setting initial values for simple models.