Don't understand the generate_and_preprocess_model_data function

[AuguB]

Hi all,

I don't think I understand what the model_builder.generate_and_preprocess_model_data function is supposed to do.

First, the function gets two inputs X and y, but the example provided in the docstring never uses those. Instead, in the example new data is generated and stored in fields self.X and self.y. Inside fit, the call to generate_and_preprocess_model_data(X,y) is immediately followed by a call to build_model(self.X,self.y). So if the example code in the docstring of generate_and_preprocess_model_data is taken, not the data profided to fit, but the data generated in generate_and_preprocess_model_data will be used to fit the model.

Second, data is not preprocessed in the example, only generated.

My suggestion would be to split this function into a generate_model_data and preprocess_model_data. That would make a lot more sense to me, but it could be that I am missing something here. Can somebody enlighten me?

Kind regards,

Stijn

[michaelraczycki]

Hey @AuguB,
The docstring example needs to be updated, thanks for pointing it out. Good example of what it's supposed to do is https://github.com/michaelraczycki/pymc-marketing/blob/main/pymc_marketing/mmm/delayed_saturated_mmm.py. In general for some models the function can be just blank, for others it should contain necessary steps to construct coords, determine presence of optional parameters etc and then generate the final dataset. It probably will be separated into 2 functions in some minor patch later on. The docstring in this case was quite tricky to show from perspective of model_builder itself since it's a mostly abstract class that doesn't include any optional input parameters and doesn't enforce any validation or preprocessing of the data passed in. Because of that the example will basically just include assignment of X and y to self.X and self.Y (the order of function call also needs updating, promise to do it asap, as fit calls build_model, and build_model calls generate_and_preprocess_model_data).

I'll keep this issue as a reminder for me to fix the docstring

[mattijsdp]

I think this question is related enough that I add it (let me know if I should create a new one):
I don't understand why generate_and_preprocess_model_data() also isn't called in load(). load() loads fit_data but in fit() the output of generate_and_preprocess_model_data() isn't saved in fit_data but rather the function's inputs.

[AuguB]

I ran into this exact issue today. Wanted to continue this thread so thanks for doing that for me. I think the solution is to put a call to generate_and_preprocess_data right before the call to build_model is made in load. The pymc_marketing example calls generate_and_preprocess_data inside build_model, but that seems superfluous since it's already called inside fit.

Edit: I see now that this could lead to problems in hierarchical models when the new data contains a subset of the groups in the train data. The newly constructed coords would not match up with those used during training. Or am I seeing this wrong?

[mattijsdp]

@AuguB I went through the same thinking that would fix it but then finding that's not quite right. At the moment I'm experimenting with adding a function inside load() which simply loads the model_coords based on the idata.*.coords

[AuguB]

Great, I'm curious about your approach.

I think that the coords problem is already partially covered. In load we have:

        model.idata = idata
        dataset = idata.fit_data.to_dataframe()
        X = dataset.drop(columns=[model.output_var])  
        y = dataset[model.output_var]
        model.build_model(X, y) #coords are built using fit data here

So the model after load has the same coords as the fit data. For now this works for me, but it might be an issue for applications where privacy is regulated, because it means that the fit data is saved along with the model.

Next, the indices of the groups in the predict data have to match with those in the fit data. We can do this with a lookup table, as is done here; county_lookup = dict(zip(mn_counties, range(counties))). This dict can be stored in the idata.attrs. Is this something you've considered?

Edit

If such a dict is used, we can also avoid the privacy issue by zero-ing out all the fit data that is stored in idata, and overwriting the coords with the dict.keys after build_model in load

[AuguB]

I made a pull request with a fix, you can check it out here