Skip to content
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

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

Open
cluhmann opened this issue Nov 11, 2024 · 11 comments
Open

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

cluhmann opened this issue Nov 11, 2024 · 11 comments
Labels
docs Improvements or additions to documentation MMM

Comments

@cluhmann
Copy link
Contributor

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 cluhmann added docs Improvements or additions to documentation MMM labels Nov 11, 2024
@cluhmann
Copy link
Contributor Author

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
Copy link
Contributor

wd60622 commented Nov 11, 2024

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:

  • Add general note in the module docstring for beta parameters in the case of curve asymptotically reaches 1.0
  • Add parameter documentation for each one of the SaturationTransformations (and Adstock classes)

Thoughts @cluhmann?

@cluhmann
Copy link
Contributor Author

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

@wd60622
Copy link
Contributor

wd60622 commented Nov 12, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

wd60622 commented Nov 12, 2024

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
Copy link
Contributor Author

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

@wd60622
Copy link
Contributor

wd60622 commented Nov 12, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

wd60622 commented Nov 12, 2024

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

@cluhmann
Copy link
Contributor Author

Yup, I think that works then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to documentation MMM
Projects
None yet
Development

No branches or pull requests

2 participants