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

Put fit_result property into ModelBuilder #1333

Closed
wd60622 opened this issue Jan 3, 2025 · 5 comments
Closed

Put fit_result property into ModelBuilder #1333

wd60622 opened this issue Jan 3, 2025 · 5 comments
Labels
CLV maintenance MMM ModelBuilder Related to the ModelBuilder class and its children

Comments

@wd60622
Copy link
Contributor

wd60622 commented Jan 3, 2025

This can be consolidated into ModelBuilder. Here it is for MMM:

@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"]

And here it is for CLV:

@property
def fit_result(self) -> Dataset:
"""Get the fit result."""
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"]

This can be done like here:

prior = create_idata_accessor(
"prior",
"Call the 'sample_prior_predictive' method first.",
)
prior_predictive = create_idata_accessor(
"prior_predictive",
"Call the 'sample_prior_predictive' method first.",
)
posterior = create_idata_accessor("posterior", "Call the 'fit' method first.")
posterior_predictive = create_idata_accessor(
"posterior_predictive",
"Call the 'sample_posterior_predictive' method first.",
)
predictions = create_idata_accessor(
"predictions",
"Call the 'sample_posterior_predictive' method with predictions=True first.",
)

@wd60622 wd60622 added ModelBuilder Related to the ModelBuilder class and its children MMM CLV maintenance and removed Needs Triage labels Jan 3, 2025
@sreekailash
Copy link
Contributor

sreekailash commented Jan 4, 2025

Hi @wd60622 - I can take it up since it was already a part of my earlier PR. Let me know if that is okay :D.

@sreekailash
Copy link
Contributor

@wd60622 Clarification on your preference - I am planning to remove the redundant fit_result method from base.py and replace the name posterior with fit_result. Would that work or do you want me to replace fit_result with posterior everywhere? Do you have a preference?

@wd60622
Copy link
Contributor Author

wd60622 commented Jan 5, 2025

Hi @wd60622 - I can take it up since it was already a part of my earlier PR. Let me know if that is okay :D.

Thanks @sreekailash
Lets quickly get the other one through fit_result property should still stay. Dont replace with posterior despite both being available. The base class will implement for all child classes

@wd60622
Copy link
Contributor Author

wd60622 commented Jan 5, 2025

The fit_replace is still used in many places

@wd60622
Copy link
Contributor Author

wd60622 commented Jan 8, 2025

Closed with #1344

@wd60622 wd60622 closed this as completed Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLV maintenance MMM ModelBuilder Related to the ModelBuilder class and its children
Projects
None yet
Development

No branches or pull requests

2 participants