Refactor mmm allow user defined priors #408
Conversation
|
This is neat. I wonder, rather than specifying dists as names so that we can serialize them, couldn't we just use Anyway, I don't think we need to chase this one down all the way and focus on this implementation instead. |
|
@MarkusSagen is this a duplicate of #397? |
|
@ricardoV94 It seems like it, yes! |
|
Can you synchronize with @cetagostini to avoid duplicate work? |
|
@MarkusSagen Hey great to see you are also working on it. Before, even moving to the unit test. Did you try to run the example notebook with these changes? Another thing, are you leaving the dims on the |
|
@cetagostini That sounds like a good idea! Leaving them fixed prevents users from having unintended consequences. The thing I got stuck on was that |
|
@MarkusSagen great, then if we are align. What do you think if we have a session during the week? Are you available to talk maybe 45 minutes, and have a session of programming where we can compile the best of the two PR and merge it on one where each of us can modify? Regarding adding dims, you can modify this in your code, adding the dimensions if you follow the self.model.register_rv(
self.beta_channel, name="beta_channel", dims=("channel",)
)or how Juan apply it here. |
|
@MarkusSagen I was able to modify everything and make it run smoothly here. If you want you can take a look, and suggest some changes if needed 🙌🏻 |
|
Have you guys seen #415? Any way to consolidate the efforts? |
| # Define custom priors | ||
| self.intercept = self._create_distribution(self.model_config["intercept"]) | ||
| self.beta_channel = self._create_distribution(self.model_config["beta_channel"]) | ||
| self.alpha = self._create_distribution(self.model_config["alpha"]) | ||
| self.lam = self._create_distribution(self.model_config["lam"]) | ||
| self.sigma = self._create_distribution(self.model_config["sigma"]) | ||
| self.gamma_control = self._create_distribution( | ||
| self.model_config["gamma_control"] | ||
| ) | ||
| self.gamma_fourier = self._create_distribution( | ||
| self.model_config["gamma_fourier"] | ||
| ) | ||
| self._process_priors( | ||
| self.intercept, | ||
| self.beta_channel, | ||
| self.alpha, | ||
| self.lam, | ||
| self.sigma, | ||
| self.gamma_control, | ||
| self.gamma_fourier, | ||
| ) |
There was a problem hiding this comment.
This should only be done in build_model
| alpha=model_config["lam"]["alpha"], | ||
| beta=model_config["lam"]["beta"], | ||
| dims=model_config["lam"]["dims"], | ||
| # FIXME: Need to add the correct dims to `beta_channel`, `alpha`, `lam`, |
There was a problem hiding this comment.
The problem here is that dims no longer affect the shape of the rv by this point.
Instead of using the approach in CLVModel which works fine because we are always building scalar distributions, use a _create_model_distribution that looks like this:
@staticmethod
def _create_model_distribution(name: str, dist: Dict, ndim: int = 0, dims = None, model: Optional[Model]=None) -> TensorVariable:
model = pm.modelcontext(model)
try:
with model:
prior_distribution = getattr(pm, dist["dist"])(name, **dist["kwargs"], dims=dims)
except AttributeError:
raise ValueError(f"Distribution {dist['dist']} does not exist in PyMC")
return prior_distributionThen you don't need to call model.register_rv, as you are already creating a model variable
|
Closing. Fixed in #397 |
4 participants
This PR intends to add custom priors to the DelayedSaturation MMM model
There is an issue however and would love for some feedback and help on how to fix it.
The DelayedSaturation MMM uses priors with dims, but the CLV model I've copied from does not. I'm currious how to add that for the priors to make the code and tests pass?
📚 Documentation preview 📚: https://pymc-marketing--408.org.readthedocs.build/en/408/