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

Enable transform _sample_curve to work with VariableFactory #1362

Merged
merged 2 commits into from
Jan 10, 2025

Conversation

lucianopaz
Copy link
Contributor

@lucianopaz lucianopaz commented Jan 10, 2025

Description

The core problem was that the transform's _sample_curve method created a Model instance that didn't have access to all of the required coordinate information. The secondary issue that I addressed was to automatically determine the expected core dimensions of the transformation's output. The way this used to work was very brittle because it relied on expecting the function_parameters to be stored in the parameters dataset. The method I used just relies on the meta information of the parameters' dims.

Related Issue

Checklist

Modules affected

  • MMM
  • CLV
  • Customer Choice

Type of change

  • New feature / enhancement
  • Bug fix
  • Documentation
  • Maintenance
  • Other (please specify):

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

@lucianopaz lucianopaz requested a review from wd60622 January 10, 2025 11:34
@github-actions github-actions bot added MMM tests media transforms Related to adstock, saturation, and media transformations model components Related to the various model components labels Jan 10, 2025
@lucianopaz lucianopaz added bug Something isn't working and removed tests media transforms Related to adstock, saturation, and media transformations model components Related to the various model components labels Jan 10, 2025
Copy link

codecov bot commented Jan 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.31%. Comparing base (38e7e00) to head (54abe72).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1362      +/-   ##
==========================================
- Coverage   95.31%   95.31%   -0.01%     
==========================================
  Files          47       47              
  Lines        4913     4912       -1     
==========================================
- Hits         4683     4682       -1     
  Misses        230      230              

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

@lucianopaz lucianopaz force-pushed the saturation_variable_factory branch from 8b6eff4 to 78b028d Compare January 10, 2025 12:19
@github-actions github-actions bot added the tests label Jan 10, 2025
@lucianopaz lucianopaz force-pushed the saturation_variable_factory branch from 78b028d to 0a56807 Compare January 10, 2025 16:49


def test_transform_sample_curve_with_variable_factory():
class Example(VariableFactory):
Copy link
Contributor

Choose a reason for hiding this comment

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

You dont need to inherit fyi

Copy link
Contributor

@wd60622 wd60622 left a comment

Choose a reason for hiding this comment

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

Thanks @lucianopaz !

@wd60622 wd60622 merged commit f80581b into pymc-labs:main Jan 10, 2025
20 checks passed
@lucianopaz lucianopaz deleted the saturation_variable_factory branch January 10, 2025 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working MMM tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mmm.components.base.Transformation._sample_curve doesn't use all of the dataset coordinates
2 participants