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

Bump pymc dependency #1269

Merged
merged 2 commits into from
Jan 8, 2025
Merged

Bump pymc dependency #1269

merged 2 commits into from
Jan 8, 2025

Conversation

ricardoV94
Copy link
Contributor

@ricardoV94 ricardoV94 commented Dec 13, 2024

Closes #726


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

@juanitorduz
Copy link
Collaborator

Thanks for this one!

Copy link

codecov bot commented Dec 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.29%. Comparing base (9684821) to head (50978df).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1269      +/-   ##
==========================================
- Coverage   95.34%   95.29%   -0.06%     
==========================================
  Files          47       47              
  Lines        4943     4889      -54     
==========================================
- Hits         4713     4659      -54     
  Misses        230      230              

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

@juanitorduz
Copy link
Collaborator

There are just two failing tests (yay!)

FAILED tests/clv/models/test_gamma_gamma.py::TestGammaGammaModel::test_spend[True]
FAILED tests/test_model_builder.py::test_fit_random_seed_reproducibility[rng]

@wd60622
Copy link
Contributor

wd60622 commented Dec 13, 2024

Closes #726

tests/test_mlflow.py Outdated Show resolved Hide resolved
@ricardoV94 ricardoV94 changed the title Bump pymc dep Bump pymc dependency Dec 13, 2024
@ricardoV94
Copy link
Contributor Author

ricardoV94 commented Dec 13, 2024

FAILED tests/clv/models/test_gamma_gamma.py::TestGammaGammaModel::test_spend[True]

AFAICT it's a seed problem. standard deviation is similar to old version, and I could get it to fail then if I changed the seed. Making tolerance higher

@ricardoV94
Copy link
Contributor Author

The second failure seems to be a regression in PyMC regarding seeding with generators: pymc-devs/pymc#7612

@ricardoV94 ricardoV94 force-pushed the bump_pymc_dep branch 2 times, most recently from 7511d19 to 2a4b943 Compare December 13, 2024 15:57
@ricardoV94
Copy link
Contributor Author

Thanks for this one!

Very specific appreciation noted :D

@juanitorduz
Copy link
Collaborator

The second failure seems to be a regression in PyMC regarding seeding with generators: pymc-devs/pymc#7612

So, do we need to wait for this to be fixed in PyMC before merging this one? Or shall we skip this test (in the meantime)?

@ricardoV94
Copy link
Contributor Author

This is buggy behavior so I would wait until the next release with the fix

@PabloRoque
Copy link
Contributor

Agua de Mayo

@ColtAllen
Copy link
Collaborator

This is buggy behavior so I would wait until the next release with the fix

The next pymc-marketing release, or next pymc release?

@ricardoV94
Copy link
Contributor Author

Agua de Mayo

What does that mean? 🤔

@ricardoV94
Copy link
Contributor Author

This is buggy behavior so I would wait until the next release with the fix

The next pymc-marketing release, or next pymc release?

Next pymc release

@PabloRoque
Copy link
Contributor

What does that mean? 🤔

Agua de Mayo may be translated as something is godsend.

Context: I got a new Apple Silicon machine last week. Making blas work has been painful. I made it work following this release of pytensor you mentioned somewhere else.

However that version is not supported in the pymc-marketing library. So indeed this PR is godsend to me.

@ColtAllen ColtAllen mentioned this pull request Dec 20, 2024
13 tasks
@aseyboldt
Copy link
Contributor

@wd60622 Is there anything holding this back?

@wd60622
Copy link
Contributor

wd60622 commented Dec 20, 2024

@wd60622 Is there anything holding this back?

@ricardoV94 mentioned non deterministic behavior with pymc version

PabloRoque added a commit to PabloRoque/pymc-marketing that referenced this pull request Jan 2, 2025
PabloRoque added a commit to PabloRoque/pymc-marketing that referenced this pull request Jan 2, 2025
PabloRoque added a commit to PabloRoque/pymc-marketing that referenced this pull request Jan 2, 2025
@ricardoV94
Copy link
Contributor Author

Hopefully it will work with https://github.com/pymc-devs/pymc/releases/tag/v5.20.0

@juanitorduz
Copy link
Collaborator

Yay 🙌!

environment.yml Outdated
@@ -15,7 +15,7 @@ dependencies:
- pydantic
- preliz
# NOTE: Keep minimum pymc version in sync with ci.yml `OLDEST_PYMC_VERSION`
- pymc>=5.12.0,<5.16.0
- pymc>=5.19.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ricardoV94 so this should be pymc>=5.20 ?

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 I didn't update yet

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove this:

def _backwards_compatiable_data_vars(model: Model) -> list[TensorVariable]:
# TODO: Remove with PyMC update
non_data = (
model.observed_RVs + model.free_RVs + model.deterministics + model.potentials
)
vars = {
key: value for key, value in model.named_vars.items() if value not in non_data
}
return list(vars.values())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I already removed it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sorry I thought it was the mlflow from the requirements. Will do

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry commented in wrong thread. Not related but was a backwards compat in the mlflow module

@github-actions github-actions bot added the mlflow label Jan 8, 2025
@ricardoV94 ricardoV94 marked this pull request as ready for review January 8, 2025 16:10
@juanitorduz juanitorduz added the major API breaking changes label Jan 8, 2025
@juanitorduz juanitorduz merged commit 8541ca5 into pymc-labs:main Jan 8, 2025
19 of 20 checks passed
PabloRoque added a commit to PabloRoque/pymc-marketing that referenced this pull request Jan 9, 2025
@ricardoV94 ricardoV94 deleted the bump_pymc_dep branch January 9, 2025 09:57
wd60622 pushed a commit that referenced this pull request Jan 14, 2025
* Add BGNBD distribution and BGNBD Random Variable

* Add BGNBD excel test

* Remove logp in terms of Potential

* Rename BGNBD -> BetaGeoNBD

* Add logp and test matching lifetimes

* Add logp param.type.ndim > 1. Add logp pt.switch. Related tests

* Add test_bg_nbd_sample_prior

* Add _distribution_new_customers and related test. Rename population_dropout|purchase_rate as dropout|purchase_rate to consolidate naming among models

* Adjust test_model_repr expected result to BetaGeoNBD instead of BGNBD

* Improve distribution_new_customer, distribution_new_customer_purchase_rate. Introduce distribution_new_customer_recency_frequency. Improve tests

* Revert @pytest.mark.slow to in test_model_convergence

* Revert distribution changes related to #1269

* Revert more changes related to #1269

* Revert BetaGeoBetaBinomRV changes

* Revert ParetoNBDRV changes

* Docstring cleanup

* Revert changes in ContContract dist

* Clean ContContract changes

* Revert deletion _supp_shape_from_params

* Remove commented chunk on fit_result. Opted for data to standardize with other CLV models

* BetaGeoNBDRV as pre-#1269 definition

* Fix test_numerically_stable_logp

* Overrride ModifiedBetaGeoModel.distribution_new_customer method

* Modify test_bg_nbd tensor parametrization to only vectors in test_bg_nbd. Passing param.type.ndim > 1 now raises NotImplementedError

* Silence mypy in 2 lines

* Adapt BetaGeoNBDRV to #1269

* Fix test_clv_fit_mcmc

* Modify sim_data to reflect the beta-distributed dropout process

* Add reference to BetaGeoNBD

* Delete _logp

* Delete commented weights param in test_bg_nbd

* Ammend BetaGeoNBD docstring

* Fix BetaGeoNBD math

* Fix test_posterior_distributions to include dropout distributions

* Fix #NUM! docstring

* Tweak sim_data

* Add co-author.

Co-authored-by: Colt Allen <[email protected]>

---------

Co-authored-by: Colt Allen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prepare for PyMC 5.16 release
6 participants