Skip to content

Conversation

@daniel-saunders-phil
Copy link
Contributor

@daniel-saunders-phil daniel-saunders-phil commented Sep 12, 2025

This is the code implementation of that positive-constrained prior that works well for hierarchies that I was telling a few folks about. Wrote up the rough draft in this post and finally got a production-ready version coded up.

We have to implement as it's own class instead of using the Prior class because it doesn't correspond to any pymc distribution and you cannot build it out of the transformations permitted by the Prior class.

I'm curious what people think of the strategy of introducing special_priors.py. I think there are quite a few odd distributions (Non-centered laplace for seasonality is sometimes way more efficient than centered laplace) that don't match the logic of the Prior class very well but would be nice to have in marketing projects. If we want to keep push that route, it might be nice to set up a base class with some abstract methods so we know what is required of all special priors.

In a future PR, I'll update the multi-dimensional notebook to use this class.


📚 Documentation preview 📚: https://pymc-marketing--1939.org.readthedocs.build/en/1939/

@github-actions github-actions bot added the tests label Sep 12, 2025
@codecov
Copy link

codecov bot commented Sep 12, 2025

Codecov Report

❌ Patch coverage is 93.67089% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.01%. Comparing base (e2ca250) to head (23e1b43).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pymc_marketing/special_priors.py 93.67% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1939      +/-   ##
==========================================
+ Coverage   92.00%   92.01%   +0.01%     
==========================================
  Files          66       67       +1     
  Lines        7853     7932      +79     
==========================================
+ Hits         7225     7299      +74     
- Misses        628      633       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@juanitorduz
Copy link
Collaborator

juanitorduz commented Sep 13, 2025

Thanks @daniel-saunders-phil ! This looks great! can you please add this new module here: https://github.com/pymc-labs/pymc-marketing/blob/main/docs/source/api/index.md so that is rendered in the docs? After that we can merge this one 🙏

@juanitorduz juanitorduz self-requested a review September 13, 2025 08:18
Copy link
Collaborator

@juanitorduz juanitorduz left a comment

Choose a reason for hiding this comment

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

Just add this new module to the docs :)

@github-actions github-actions bot added the docs Improvements or additions to documentation label Sep 14, 2025
@cetagostini
Copy link
Contributor

My two recommendations would be:

  1. Maybe better say "Custom" instead of "Special"?
  2. If you want (Can be one issue and another PR), I feel separate to dict, from dict, sample prior should be easy in a class, then inherit, and only define create_variable.

I feel we could get from pymc-extras all the base class from prior even, and only define the custom logic in create variable, or not? @daniel-saunders-phil

cc: @williambdean

@daniel-saunders-phil
Copy link
Contributor Author

My two recommendations would be:

  1. Maybe better say "Custom" instead of "Special"?
  2. If you want (Can be one issue and another PR), I feel separate to dict, from dict, sample prior should be easy in a class, then inherit, and only define create_variable.

I feel we could get from pymc-extras all the base class from prior even, and only define the custom logic in create variable, or not? @daniel-saunders-phil

cc: @williambdean

Thanks Carlos!

  1. I think "custom" might add a bit of confusion. In other places on the docs, we use custom prior to refer to anything other than default priors. The "How we compare" page lists custom priors as a feature we already have. So I won't want new users to come clicking on custom priors, expecting something else.

  2. Yeah it's a good idea but it should be follow up PR. We need to refactor on pymc-extras a bit before we could inherit methods like to_dict. The LogNormalPositiveParam method is not exactly the same as the Prior class's method.

@daniel-saunders-phil
Copy link
Contributor Author

Added this issue to track future changes around inheritance #1941

@juanitorduz
Copy link
Collaborator

Thanks @daniel-saunders-phil, I think this is a great contribution and we can showcase it in some example notebooks! We can iterate in the class and structures of these distributions in the future 💪

@juanitorduz juanitorduz merged commit 6b30c10 into main Sep 15, 2025
33 checks passed
@juanitorduz juanitorduz deleted the special_priors branch September 15, 2025 19:36
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 enhancement New feature or request Prior class tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants