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

Allow MAP fitting for CLV models #130

Merged
merged 4 commits into from
Jan 27, 2023
Merged

Allow MAP fitting for CLV models #130

merged 4 commits into from
Jan 27, 2023

Conversation

ricardoV94
Copy link
Contributor

Closes #99

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@ricardoV94 ricardoV94 added enhancement New feature or request CLV labels Jan 23, 2023
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@@ -6,7 +6,6 @@ addopts = [
"-v",
"--strict-markers",
"--strict-config",
"--cov=mmm",
"--cov=pymmmc",
"--cov=pymc_marketing",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it should be pymc-marketing either :P

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe is pymc-marketing ...the coverage and tests seem running as expected on this PR right?

Copy link
Contributor Author

@ricardoV94 ricardoV94 Jan 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this addopts do? Why was the previous mmm/ pymmmc fine, even after we changed the package name?

@ricardoV94 ricardoV94 changed the title Add fit_MAP to CLV model Add fit_MAP to CLV models Jan 23, 2023
with self.model:
self._fit_result = pm.sample(*args, **kwargs)
return self._fit_result

def fit_MAP(self, **kwargs):
"""Find model maximum a posterior using scipy optimizer"""
Copy link
Collaborator

@ColtAllen ColtAllen Jan 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does pm.find_MAP() use scipy under the hood? Also, looks like 'posteriori' was misspelled here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it uses scipy under the hood. Thanks for flagging the typo

@ColtAllen
Copy link
Collaborator

Apart from the docstring typo, this looks good! We floated the idea of rolling both pm.sample() and pm.find_MAP() into a single fit() method, but these return InferenceData objects that are quite different, so it's probably best to keep them separated.

@ricardoV94
Copy link
Contributor Author

ricardoV94 commented Jan 24, 2023

Apart from the docstring typo, this looks good! We floated the idea of rolling both pm.sample() and pm.find_MAP() into a single fit() method, but these return InferenceData objects that are quite different, so it's probably best to keep them separated.

They return very similar objects, so that the summary statistic methods work the same. We could use the single fit method with a keyword argument:

model.fit(method=["mcmc"|"map"|"vi"])

Do people like that more? I think I like it more.

@ColtAllen
Copy link
Collaborator

They return very similar objects, so that the summary statistic methods work the same. We could use the single fit method with a keyword argument:

model.fit(method=["mcmc"|"map"|"vi"])

Do people like that more? I think I like it more.

If the return objects are functionally equivalent, then this gets my vote.

I only briefly tested ADVI when I started working on BTYD, but the posteriors it generated did not impress me.

@zwelitunyiswa
Copy link

Bambi has an "inference_method" parameter in its .fit function. In mcmc, and vi, it also allows "nuts_numpyro" and "nuts_blackjax". Will the .fit function here allow sampling using jax too?

@ricardoV94
Copy link
Contributor Author

The PyMC sample function will soon allow to choose one of those variations of mcmc via a kwarg so that should be available automatically here:

https://github.com/pymc-devs/pymc/pull/6422/files

@zwelitunyiswa
Copy link

@ricardoV94 very cool. Thanks for the clarification.

1. Remove last plot which made sense when using `distribution_customer_spend` but not `expected_customer_spend`.
2. Show example of MAP fitting
@ricardoV94
Copy link
Contributor Author

Updated to use a keyword in the user facing fit method

@ricardoV94 ricardoV94 merged commit f6ad349 into main Jan 27, 2023
@ricardoV94 ricardoV94 changed the title Add fit_MAP to CLV models Allowing MAP fitting for CLV models Feb 8, 2023
@ricardoV94 ricardoV94 changed the title Allowing MAP fitting for CLV models Allow MAP fitting for CLV models Feb 8, 2023
@ricardoV94 ricardoV94 deleted the add_find_MAP branch February 22, 2023 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLV enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider using "penalized" priors by default in CLV models
4 participants