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.

Sign up for GitHub

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

Jump to bottom

Remove duplicate attrs/functions from MMM #1336

Merged
merged 2 commits into from
Jan 5, 2025
Merged

Remove duplicate attrs/functions from MMM #1336

merged 2 commits into from
Jan 5, 2025
+6 −31

Conversation

sreekailash
Copy link
Contributor

@sreekailash sreekailash commented Jan 4, 2025 •
edited by wd60622
Loading

Description

Here are the changes I have made :

  1. I have removed the unnecessary functions in the base.py file that is already being inherited from the ModelBuilder class.
  2. Replaced the "fit_result" which was the method name used in the base.py definition for posterior with "posterior" in all the relevant places in mmm.py and base.py.

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--1336.org.readthedocs.build/en/1336/

@github-actions github-actions bot added the MMM label Jan 4, 2025
@sreekailash
Copy link
Contributor Author

@wd60622 - I have created a draft PR for issue #1332. Since this is my first PR, would appreciate some help/support from you. Feel free to let me know if there is anything else that I need to do from my end. Appreciate any critics/thoughts.

Thanks

wd60622
wd60622 reviewed Jan 4, 2025
View reviewed changes
pymc_marketing/mmm/base.py Outdated
Comment on lines 292 to 297
@property
def fit_result(self) -> Dataset:
"""Get the posterior data."""
if self.idata is None or "posterior" not in self.idata:
raise RuntimeError("The model hasn't been fit yet, call .fit() first")
return self.idata["posterior"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep this for now and leave it for the scope of #1333 after this

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 did it and have updated the PR. I can work on #1333 after this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect. Thanks
Just waiting for the CI/CD of this one then will merge in

@wd60622
Copy link
Contributor

wd60622 commented Jan 4, 2025

Thanks for the PR @sreekailash

Your setup seems good so far. I've updated the base branch so you will have to do a git pull
It seems like you did everything from this guide which is great.

Just have comment about the PR scope and trying to keep it even smaller 😄

@github-actions github-actions bot added ModelBuilder Related to the ModelBuilder class and its children good first issue Good for newcomers . Doesn't require extensive knowledge of the repo and package labels Jan 4, 2025
@sreekailash
Copy link
Contributor Author

sreekailash commented Jan 4, 2025 •
edited
Loading

Thanks for the suggestion @wd60622 . I will keep that in mind going forward. I have updated the PR. Let me know how this looks. Appreciate the support.

@wd60622 wd60622 marked this pull request as ready for review January 4, 2025 15:58
@codecov Codecov
Copy link

codecov bot commented Jan 4, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.35%. Comparing base (c989eeb) to head (0e8f073).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #1336   +/-   ##
=======================================
  Coverage   95.34%   95.35%           
=======================================
  Files          47       47           
  Lines        4963     4948   -15     
=======================================
- Hits         4732     4718   -14     
+ Misses        231      230    -1

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

@wd60622
Copy link
Contributor

wd60622 commented Jan 4, 2025

Hi @sreekailash can you fix the failing tests. The error message is now different

@wd60622
Copy link
Contributor

wd60622 commented Jan 4, 2025

You should be able to change the "match" based on the new message

@sreekailash
Copy link
Contributor Author

@wd60622 Can you check now?

wd60622
wd60622 approved these changes Jan 5, 2025
View reviewed changes
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 for the contributions @sreekailash !

@wd60622 wd60622 merged commit 8d808f3 into pymc-labs:main Jan 5, 2025
20 checks passed
@wd60622
Copy link
Contributor

wd60622 commented Jan 5, 2025

Still interested in the other issue?

@sreekailash
Copy link
Contributor Author

@wd60622 . Yes! Let me work on the fit_result and will raise a draft PR. Thanks for the support! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wd60622 wd60622 wd60622 approved these changes

Assignees
No one assigned
Labels
good first issue Good for newcomers . Doesn't require extensive knowledge of the repo and package MMM ModelBuilder Related to the ModelBuilder class and its children
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove duplicated attrs from MMM
2 participants
@sreekailash @wd60622