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

Posterior predictive sampling fails when seasonality is true and pm.set_data() is used #1218

Open
cluhmann opened this issue Nov 21, 2024 · 6 comments
Labels
bug Something isn't working MMM

Comments

@cluhmann
Copy link
Contributor

cluhmann commented Nov 21, 2024

To quote from the issue highlighted here:

It seems that the build_model, in 0.10.0, adds the dayofyear variable if yearly_seasonality is passed (not None) when the model is instantiated...but when sample_posterior_predictive is called, it crashes with the same KeyError you encountered [KeyError: 'dayofyear']. What happens in the method is that it first enriches the passed data with a dayofyear column, and then passes the enriched data to pm.set_data() which iterates over the columns of the passed data (including dayofyear!) and tries to assign the column’s data to the respective variable – which throws the mentioned KeyError since the variable is not there.

@cluhmann cluhmann added bug Something isn't working MMM labels Nov 21, 2024
@JnsLns
Copy link

JnsLns commented Nov 21, 2024

Note that at the time of writing it's not entirely clear from the linked discussion whether OP's problem (KeyError for dayofyear when calling sample_posterior_predictive) is indeed a bug or stems from some other cause.

The quote comes from my response there and just describes why I saw the same exception when loading a model from 0.7.0 in 0.10.0. Whether that is a bug depends on whether compatibility between these versions' models is intended or not, I suppose.

@wd60622
Copy link
Contributor

wd60622 commented Nov 21, 2024

Note that at the time of writing it's not entirely clear from the linked discussion whether OP's problem (KeyError for dayofyear when calling sample_posterior_predictive) is indeed a bug or stems from some other cause.

The quote comes from my response there and just describes why I saw the same exception when loading a model from 0.7.0 in 0.10.0. Whether that is a bug depends on whether compatibility between these versions' models is intended or not, I suppose.

Hi @JnsLns
Do you experience this when saving a 0.10.0 MMM with the desired config then loading again? Ie not having asset from previous version

@JnsLns
Copy link

JnsLns commented Nov 21, 2024

@wd60622 I do not experience it when creating/fitting the model in 0.10.0. I haven't had the opportunity to try what happens when I load that model, though I suspect it will work. Note that I serialize models without using the inbuilt save/load functionality, so it does not have to do with those.

In my opinion, as concerns loading models, the question boils down to whether backward compatibility is intended. Why the error appears seems clear: 0.7.0 does not create a model variable that 0.10.0 expects.

But I suspect what prompted the discussion linked by @cluhmann had a different (but maybe related) origin.

@wd60622
Copy link
Contributor

wd60622 commented Nov 21, 2024

We use the save and load methods to check backwards compatibility. I am hearing you were using another way to load model, is that correct?

@JnsLns
Copy link

JnsLns commented Nov 22, 2024

That's correct, I use dill to serialize the model. I see that ModelBuilder.load() essentially regenerates the model from inference data, calling build_model in the process, which also creates the dayofyearvariable. So it's clear that this establishes backward compatibility, which my simple deserialization does not.

I could probably simply save the inference data from my 0.7.0 serialized model to netcdf and pass it to the load method to recreate the model in 0.10.0. I may try later.

@wd60622
Copy link
Contributor

wd60622 commented Nov 22, 2024

Yes, I am sure you can port the idata with a little wrangling then attach to new model object. Then save again.

However, I am not exactly sure what you did, so it is hard to say for sure

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
Projects
None yet
Development

No branches or pull requests

3 participants