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

Implement gamma-gamma model and base CLV API #61

Merged
merged 2 commits into from
Nov 7, 2022
Merged

Conversation

ricardoV94
Copy link
Contributor

@ricardoV94 ricardoV94 commented Oct 26, 2022

@juanitorduz
Copy link
Collaborator

juanitorduz commented Oct 28, 2022

@ricardoV94 this is very cool! It works very nicely! 🚀 I really like the fact you explain the PYMC implementation and its limitations. If I understand correctly when conditioning on the mean purchase per user there is no way around using pm.Potential right?

For applications, you usually have a lot of users (millions). I was wondering if we should allow variational inference estimation of the parameters (maybe pathfinder from pymc-experimental?) Do you see any problem with this?

pymmmc/clv/models/gamma_gamma.py Outdated Show resolved Hide resolved
pymmmc/clv/models/gamma_gamma.py Outdated Show resolved Hide resolved
pymmmc/clv/models/gamma_gamma.py Outdated Show resolved Hide resolved
pymmmc/clv/models/gamma_gamma.py Show resolved Hide resolved
@juanitorduz
Copy link
Collaborator

I left some suggestions regarding documentation (doctstrings and references of the equations). Still, if you are happy we can merge this first iteration and create tickets about the docs.

@ricardoV94
Copy link
Contributor Author

Not without a single test! :D

Copy link
Contributor

@larryshamalama larryshamalama left a comment

Choose a reason for hiding this comment

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

Great work @ricardoV94! This API looks very nice and makes more sense than having the model as a distribution class. I left some comments and questions, as usual :)

pymmmc/clv/models/basic.py Outdated Show resolved Hide resolved
pymmmc/clv/models/gamma_gamma.py Show resolved Hide resolved
pymmmc/clv/models/gamma_gamma.py Show resolved Hide resolved
@ricardoV94 ricardoV94 force-pushed the gamma_gamma branch 3 times, most recently from ede9003 to bd79058 Compare November 2, 2022 16:59
@ricardoV94 ricardoV94 changed the title [WIP] Implement gamma-gamma model and base CLV API Implement gamma-gamma model and base CLV API Nov 4, 2022
@ricardoV94
Copy link
Contributor Author

Ready for a final review

pymmmc/clv/__init__.py Show resolved Hide resolved
pymmmc/clv/models/gamma_gamma.py Show resolved Hide resolved
@ricardoV94 ricardoV94 deleted the gamma_gamma branch November 7, 2022 08:37
@ricardoV94 ricardoV94 added the enhancement New feature or request label Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants