pymc-marketing icon indicating copy to clipboard operation
pymc-marketing copied to clipboard

Pull out `_create_likelihood_distribution` method as function

Open williambdean opened this issue 1 year ago • 2 comments
trafficstars

I think this would be better suited as a function outside of the MMM class.

https://github.com/pymc-labs/pymc-marketing/blob/9480869b9b9e03774a081eb7e5f2da96f1bb95a4/pymc_marketing/mmm/delayed_saturated_mmm.py#L152-L248

Same thoughts on _get_distribution

williambdean avatar Feb 27 '24 18:02 williambdean

maybe in mmm/utils.py?

juanitorduz avatar Feb 29 '24 21:02 juanitorduz

Why do you think it's better outside? Will you use it outside of a Model class?

ricardoV94 avatar Feb 29 '24 21:02 ricardoV94

I just brought it up since #267 isn't very clear in my eyes and a custom MMM with a config like ours would require this functionality. It can stay as a method of a class but I don't think it hurts to have it independent of a class as well

related to #708

williambdean avatar Jun 03 '24 13:06 williambdean

Are nested priors the primary motivation for this?

That was my understanding when I commented on https://github.com/pymc-labs/pymc-marketing/issues/708. If so, I think this should be relocated to the base ModelBuilder class so it's also supported in the CLV module.

If there are no other anticipated uses for this function apart from nested/heirarchical priors, I also think it should be renamed to something more meaningful.

ColtAllen avatar Jun 03 '24 15:06 ColtAllen

Just related. Not the motivator.

I just oppose putting it in ModelBuilder method because I use this elsewhere as functions that I had to copy. I haven't fully warmed up to the ModelBuilder yet / my usecase doesn't work with the current structure of it

williambdean avatar Jun 03 '24 15:06 williambdean

Realizing also that this list of distributions could have Beta as it has as mu parameter

EDIT: This is expanded in the Prior PR

williambdean avatar Jun 04 '24 07:06 williambdean