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

New spends forward pass #456

Merged
merged 41 commits into from
Mar 8, 2024
Merged

New spends forward pass #456

merged 41 commits into from
Mar 8, 2024

Conversation

wd60622
Copy link
Contributor

@wd60622 wd60622 commented Dec 7, 2023

Current code addressing #451. Have a look @carlosagostini

I've split it into two methods:

  • generation the data for the plot
  • plotting of the data

I'd like to hear feedback on the arguments for the generation. Specifically, the spends and the one_time
Currently, it takes a np.ndarray that would be a raw spend for each channel (error if it doesn't match) and will ultimately transform it to a (max_lag, n_channels) ndarray with values depending on the one_time argument.
One time:

  • True -> spends are the first and all rest are zeros in the (max_lag, n_channels)
  • False -> spends for each channel are for each of the
    This locks into a bit of assumption of how this would be used. Maybe, the spends could be (n_channels, ) or (max_lags, n_channels) to reduce the need of one_time and simplify the method.
    Would like to hear how you might use it.

The method returns the untransformed spends although named _contributions. The inverse transformation only happen in the plotting method, but it maybe good to have the inverse transformations happening that first method.
I think this is a good case to remove the target transformation from the sklearn pipeline so it can be part of the forward pass of contributions @ricardoV94

EDIT: The inverse transformation is done in the new_spend_contributions method as well


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

@wd60622 wd60622 marked this pull request as draft December 7, 2023 06:52
Copy link

codecov bot commented Dec 7, 2023

Codecov Report

Attention: Patch coverage is 91.80328% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 91.09%. Comparing base (2f2d814) to head (a523f00).
Report is 41 commits behind head on main.

Files Patch % Lines
pymc_marketing/mmm/delayed_saturated_mmm.py 91.11% 4 Missing ⚠️
pymc_marketing/mmm/utils.py 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #456      +/-   ##
==========================================
- Coverage   91.24%   91.09%   -0.15%     
==========================================
  Files          21       21              
  Lines        2044     2056      +12     
==========================================
+ Hits         1865     1873       +8     
- Misses        179      183       +4     

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

@wd60622
Copy link
Contributor Author

wd60622 commented Dec 7, 2023

Got the prior functionality working

prior-and-posterior-support

The inverse_transform could be used if the model was fit. Might be good to toggle inverse_transform even for the posterior. Any thoughts here?

@wd60622
Copy link
Contributor Author

wd60622 commented Jan 12, 2024

Will add the transformation back to original space and continue with this PR after #482

juanitorduz and others added 22 commits January 27, 2024 19:27
* Optimisation in customer_lifetime_value when discount_rate == 0

cf #467

* Update utils.py
* clv_summary adapted into rfm_summary

* added clv_summary with warning

* moved dataset from testing folder
updates:
- [github.com/astral-sh/ruff-pre-commit: v0.1.11 → v0.1.14](astral-sh/ruff-pre-commit@v0.1.11...v0.1.14)
- [github.com/pre-commit/pre-commit-hooks: v3.2.0 → v4.5.0](pre-commit/pre-commit-hooks@v3.2.0...v4.5.0)
* add baselined saturation with test and plots

* refactor docs

* add the reparam

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* verify parametrization is equivalent under change of baseline

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* add a note for setting x0

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* make it clear how r_ref is calculated

* fix typo

* fix docstrings

* improve test by making sure transform is gives identical saturation and cac0

* add comment in the docstring

* add blank line in the code-block

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
* Add support for string mode args

* Swap before and after and make mode explicit

* Use Union due Python 3.9

* Style
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

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.

This looks very nice! I added some minor suggestions on the docstrings to clarify this better for the end-user. WDYT?

pymc_marketing/mmm/delayed_saturated_mmm.py Show resolved Hide resolved
pymc_marketing/mmm/delayed_saturated_mmm.py Outdated Show resolved Hide resolved
pymc_marketing/mmm/delayed_saturated_mmm.py Outdated Show resolved Hide resolved
pymc_marketing/mmm/delayed_saturated_mmm.py Show resolved Hide resolved
pymc_marketing/mmm/delayed_saturated_mmm.py Outdated Show resolved Hide resolved
pymc_marketing/mmm/delayed_saturated_mmm.py Show resolved Hide resolved
pymc_marketing/mmm/delayed_saturated_mmm.py Outdated Show resolved Hide resolved
pymc_marketing/mmm/delayed_saturated_mmm.py Outdated Show resolved Hide resolved
@wd60622
Copy link
Contributor Author

wd60622 commented Feb 11, 2024

I've addressed all the feedback, @juanitorduz
Thanks!

I've separated out the "new_spend" creation into a function and added some more to the docstrings so hopefully that brings in some clarity.

Let me know what you think!

@juanitorduz
Copy link
Collaborator

juanitorduz commented Feb 15, 2024

@wd60622 looks great! There is one test failing, though. If you fix it, we can merge this one :)

@wd60622
Copy link
Contributor Author

wd60622 commented Feb 16, 2024

Have the test passing locally but not in the action 😢

Running the action with 3.11 with the latest pymc to see if the pymc version has an impact. The test depends on the sample_prior_predictive method. Not sure if that has change

Might just mock the idata for all of these tests with the plots and hope giving a seed will keep the results the same

@wd60622
Copy link
Contributor Author

wd60622 commented Mar 6, 2024

Trying to mock the prior in the model.idata
Hope that that works. If not, I will pull out the plotting into a function and tests on some fixed data.

If all else fails, these are tests against the plotting images and can maybe be omitted. Thoughts? @juanitorduz @ricardoV94

EDIT: They seem to be failing again... This tests are working locally for me. Honestly super hard to check what's going on on the test comparison. Any idea if tests files are stored off? i.e. log here
Thought I'd give this pytest-mpl a try though. Inspired by this #355

@juanitorduz
Copy link
Collaborator

@wd60622 I am getting the same error as in matplotlib/pytest-mpl#69 . Can you please add markers =["mpl_image_compare"] to pyproject.toml as in

[tool.pytest.ini_options]
addopts = [
    "-v",
    "--strict-markers",
    "--strict-config",
    "--cov=pymc_marketing",
    "--cov-report=term-missing",
    "--color=yes",
]
markers =["mpl_image_compare"]
filterwarnings = ["ignore::DeprecationWarning:bokeh.core.property.primitive:37"]
testpaths = "tests"

@wd60622
Copy link
Contributor Author

wd60622 commented Mar 8, 2024

Failed again even after using fresh environment 😢
Would you be able to check one more time @juanitorduz

@juanitorduz
Copy link
Collaborator

Failed again even after using fresh environment 😢 Would you be able to check one more time @juanitorduz

I suggest we make sure the plot runs (as other plots tests) and create an issue to add this matplotlib testing framework. I would not delay this feature because of this :)

@wd60622
Copy link
Contributor Author

wd60622 commented Mar 8, 2024

Failed again even after using fresh environment 😢 Would you be able to check one more time @juanitorduz

I suggest we make sure the plot runs (as other plots tests) and create an issue to add this matplotlib testing framework. I would not delay this feature because of this :)

Sounds great!

@juanitorduz juanitorduz merged commit d5331be into pymc-labs:main Mar 8, 2024
9 checks passed
@juanitorduz
Copy link
Collaborator

Thanks @wd60622 !

@wd60622
Copy link
Contributor Author

wd60622 commented Mar 8, 2024

Thanks @wd60622 !

Thank you for the review!

@wd60622 wd60622 added enhancement New feature or request MMM labels Mar 15, 2024
@wd60622 wd60622 deleted the new-spends branch April 8, 2024 21:06
wd60622 added a commit that referenced this pull request Apr 8, 2024
* current status as method

* format

* Update version.txt

* Implement different convolution modes (#454)

* Add PR template

* Update pull_request_template.md

* Fix issues in index example

* Update .pre-commit-config.yaml

* Update .pre-commit-config.yaml

* move from other PR

* put legend on side

* Optimisation in customer_lifetime_value when discount_rate == 0 (#468)

* Optimisation in customer_lifetime_value when discount_rate == 0

cf #467

* Update utils.py

* Update README.md

* add support for pre-commit-ci

* add isort

* modify autosummary templates

* Rename `clv_summary` to `rfm_summary` and extend functionality (#479)

* clv_summary adapted into rfm_summary

* added clv_summary with warning

* moved dataset from testing folder

* Update version.txt

* improve ruff

* [pre-commit.ci] pre-commit autoupdate

updates:
- [github.com/astral-sh/ruff-pre-commit: v0.1.11 → v0.1.14](astral-sh/ruff-pre-commit@v0.1.11...v0.1.14)
- [github.com/pre-commit/pre-commit-hooks: v3.2.0 → v4.5.0](pre-commit/pre-commit-hooks@v3.2.0...v4.5.0)

* resolve conflict

* Add baselined saturation (#498)

* add baselined saturation with test and plots

* refactor docs

* add the reparam

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* verify parametrization is equivalent under change of baseline

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* add a note for setting x0

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* make it clear how r_ref is calculated

* fix typo

* fix docstrings

* improve test by making sure transform is gives identical saturation and cac0

* add comment in the docstring

* add blank line in the code-block

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* Swap Before and After convolution modes as per #489 (#501)

* Add support for string mode args

* Swap before and after and make mode explicit

* Use Union due Python 3.9

* Style

* resolve conflict

* add dim_name arg

* add seed to tests and test methods

* add slice as type hint

* use slice in docstring

* defaults to mean for each channel

* add non-negative check

* ax as last arg

* change weeks -> time

* parameterize quantiles

* separate out and add to docs

* rerun the baseline images

* mock the prior

* add new images from latest env

* migrate to toml instead of ci/cd

* test only is axes

* remove the images

---------

Co-authored-by: Juan Orduz <[email protected]>
Co-authored-by: Abdalaziz Rashid <[email protected]>
Co-authored-by: Ricardo Vieira <[email protected]>
Co-authored-by: Ricardo Vieira <[email protected]>
Co-authored-by: vincent-grosbois <[email protected]>
Co-authored-by: juanitorduz <[email protected]>
Co-authored-by: Oriol (ProDesk) <[email protected]>
Co-authored-by: Colt Allen <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Maxim Kochurov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request MMM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants