pymc-labs / pymc-marketing

Bayesian marketing toolbox in PyMC. Media Mix (MMM), customer lifetime value (CLV), buy-till-you-die (BTYD) models and more.
https://www.pymc-marketing.io/
Apache License 2.0
705 stars 198 forks source link

`LogisticSaturation` class does not document all of it's parameters #1188

Open cluhmann opened 4 days ago

cluhmann commented 4 days ago

The logistic saturation function seems to come in 2 varieties. There is the LogisticSaturation class defined here and then there is the logistic_saturation function defined here. The former has no documentation of parameters (though there is a brief vignette?) and instead says it is a "Wrapper around logistic saturation function" and see the documentation on the latter. The problem is that there the former takes a parameter (beta) that is not found anywhere in the latter. And the vignette doesn't help because it passes in no parameter values. At best, this seems very confusing.

Are there other functions and wrappers that behave like this?

cluhmann commented 4 days ago

Looking at it again, I realize that the code in the docstring isn't a vignette, but rather generates a plot. I don't think we need this in the LogisticSaturation class docstring as it a) doesn't show off the invoking code (and thus is totally mysterious) and b) is redundant with the (much prettier and more informative) plots in the logistic_saturation function doc string. If the plot showed off plots for various values of beta and lam that would be unique and potentially useful, but right now it's not.

wd60622 commented 4 days ago

All of the Saturation transformations get a scaling parameter beta in order to match with the Menten alpha parameter. It might be documented in the module since it applies to most of them.

If you have a suggestion of how to improve it, please let me know.

EDIT: I took a look at the module docs and don't see something for that. I think we should re-document the parameters required for each one of the transformations so they are seen in this context as well.

My suggestion is:

Thoughts @cluhmann?

cluhmann commented 3 days ago

So if there's a note added to the module, where would that show up?

wd60622 commented 3 days ago

So if there's a note added to the module, where would that show up?

That was part of my edit and realization; it is not there. I added to my action items

cluhmann commented 3 days ago

So if there's a note added to the module, where would that show up?

That was part of my edit and realization; it is not there. I added to my action items

Yeah, I was asking where it would show up if it was added. I just don't have a sense of where I would find it in the docs if this issue was addressed in the way you are suggesting.

wd60622 commented 3 days ago

I was thinking about the module docstring. So in pymc_marketing.mmm.components.saturation.py at the top. Do you have any other suggestions?

cluhmann commented 3 days ago

Sure, and where would it appear in the docs if it was there? Here?

wd60622 commented 3 days ago

Sure, and where would it appear in the docs if it was there? Here?

Yes. Somewhere in that top section. Sound reasonable home for it?

cluhmann commented 3 days ago

I guess, but if people find their way to one of the constituent saturation components (looking for details about the LogisticSaturation), then the use of beta will still not be evident. So how do we alert people to the actual set of parameters of the individual saturation components?

wd60622 commented 3 days ago

Yeah, thats why I have the second item to add documentation for each transformation class

cluhmann commented 2 days ago

Yup, I think that works then.