-
Notifications
You must be signed in to change notification settings - Fork 242
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
excluded _prior suffix from CLV models #1498
base: main
Are you sure you want to change the base?
excluded _prior suffix from CLV models #1498
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1498 +/- ##
=======================================
Coverage 92.58% 92.59%
=======================================
Files 52 52
Lines 6043 6048 +5
=======================================
+ Hits 5595 5600 +5
Misses 448 448 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Thanks for the PR @arthurmello
We should provide a deprecation for this. Likely in the clv/basic class. Maybe we can check for any model_config keys with "_prior" suffix then remove while having a warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we write a test to make sure that save / load is backwards compatible in this scenario
Sure, do you mean a test that ensures that models saved with the old configuration can still be loaded? def test_backward_compatibility_with_old_config(self, mocker):
old_model_config = {
"alpha_prior": ...
"r_prior": ...
}
model = CLVModelTest(model_config=old_model_config)
mocker.patch("pymc.sample", mock_sample)
model.fit(...)
model.save("old_model_config_test")
loaded_model = CLVModelTest.load("old_model_config_test")
assert loaded_model.model_config == {
"alpha": ...,
"r": ...
} |
Yeah, something like that. Dont need to sample. Just load and make sure that a method that uses the posterior still works We have an issue if the built model has variables without the prefix and posterior samples do have the prefix. We might have to rename the variables upon load as well |
tests/clv/models/test_basic.py
Outdated
def test_backward_compatibility_with_old_config(self): | ||
old_model_config = { | ||
"alpha_prior": Prior("Weibull", alpha=2, beta=10), | ||
"r_prior": Prior("Weibull", alpha=2, beta=1), | ||
} | ||
model = CLVModelTest(model_config=old_model_config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's test that there is a warning
tests/clv/models/test_basic.py
Outdated
} | ||
model = CLVModelTest(model_config=old_model_config) | ||
model.fit(tune=0, chains=2, draws=5) | ||
model.save("old_model_config_test") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't testing for loading from an old saved model. The idata.posterior will have the suffix. We want to check for that suffix in the posterior will not break using the model in the future
Description
Removed the
_prior
suffix from parameters of CLV model config + adjacent files (tests, notebooks, etc.)Related Issue
Checklist
pre-commit.ci autofix
to auto-fix.📚 Documentation preview 📚: https://pymc-marketing--1498.org.readthedocs.build/en/1498/