Rename fit_method parameter

[wd60622]

This might conflict with one of the parameters of pm.sample but I find model.fit(fit_method="something") redundant

pymc-marketing/pymc_marketing/clv/models/basic.py

Lines 97 to 99 in 7dfa955

	def fit( # type: ignore
	self,
	fit_method: str = "mcmc",

I think model.fit(method="something") reads a bit better. Maybe model.fit(algorithm="something") if there is a conflict.

Thoughts @ColtAllen

[ColtAllen]

@wd60622 this crossed my mind as well when I worked on #1266. I should preface by saying the "fit_method" convention is over two years old and changing it could break the codebases of many, many users, but I never liked that param name either.

I don't want to use "method" because of the standard Python definition for that term (i.e. any function written inside a class object).

sampler could be a good alternative, but model.fit(sampler="map") would really trigger some Bayesians 😅 on that note, is MAP not supported in ModelBuilder?

model.fit(optimizer="mcmc") is another option. Bayesian sampling has many theoretical parallels with deterministic optimization, but that's a separate family of algorithms, and stochastic optimization itself is very different.

Perhaps I'm overthinking this. What are your thoughts?

[wd60622]

I don't want to use "method" because of the standard Python definition for that term (i.e. any function written inside a class object).

I am not against "method" as it is not a keyword in python

sampler could be a good alternative, but model.fit(sampler="map") would really trigger some Bayesians 😅 on that note, is MAP not supported in ModelBuilder?

That seems to fall apart with map

For me, optimizer doesn't match with mcmc then.

@wd60622 this crossed my mind as well when I worked on #1266. I should preface by saying the "fit_method" convention is over two years old and changing it could break the codebases of many, many users, but I never liked that param name either.

I'm not concerned about this. We can just deprecate it. Even if it was a keyword argument, we can just support both for a time being and have a warning. Juan and I discussed a 2 release cycle deprecation a while back with some MMM items. We could do that here as well.

For instance,

def fit(self, method: str = "mcmc", fit_method: str | None = None, **kwargs): 
    if method and fit_method: 
        raise ValueError("Both method and fit_method cannot be used. Use method in the future")

    if fit_method is not None: 
        warnings.warn("This will be deprecated in two releases. Use method instead.", DeprecationWarning)
        method = fit_method

    ...


# This stays the same
model.fit("map")
# Gets a deprecation warning
model.fit(fit_method="map")