Open hongkahjun opened 5 years ago
@hongkahjun unfortunately this is a somewhat deep issue. When writing the new terms
functionality, i diverged from sklearn's requirement that the estimator instance's parameters are not changed after looking at data (even though the coefficients are allowed to change).
This will require a deeper fix right now :/
Getting what looks like the same problem when using LinearGAM
with sklearn's TransformedTargetRegressor
.
Admittedly knowing absolutely nothing about the motivations for the changes you made, it seems like diverging from such an important package's requirements would be a bad idea? I know that if I can't get it to play nice with my company's existing sklearn infrastructure I'm probably going to have to abandon it.
Is there an older version that keeps to sklearn's requirements?
@dswah
I see two solutions to this issue:
According to the SkLearn developer guide since the terms depend on the data it could be argued that they are "estimated parameters". This would allow for self.terms_
to be assigned from self.terms
in _validate_data_dep_params
and then self.terms_
to be used throughout. This would, I believe, follow the contract set out by SkLearn.
Another option would be to overload self.get_params()
to return the terms
that the estimator was initialized with rather than self.terms
?
Please, let me know if either of these options is preferable and I will happily throw together a PR to fix this issue. Also, let me know if I've missed something!
Actually, having looked at this further this does not seem to have anything at all to do with terms
it instead occurs because the default value of the callbacks
keyword argument for every GAM estimator is a list
. This tends to be a big no no in Python as lists are mutable and so leads to confusing situations where it is possible to change the default value of a keyword argument.
I believe this was the root of this bug. When I instead apply the changes is #267 GridSearchCV succeeds with no issues.
Could you please look at this PR and see it is an acceptable fix?
It seems this can be simply fixed by adding "callbacks=callbacks," to line 2267 of pygam.py.
Also having this issue
Is this not fixed yet?
It seems this can be simply fixed by adding "callbacks=callbacks," to line 2267 of pygam.py.
here is complete correction to pygam.py (line 2461):
super(LinearGAM, self).init( callbacks=callbacks, terms=terms, distribution=NormalDist(scale=self.scale), link='identity', max_iter=max_iter, tol=tol, fit_intercept=fit_intercept, verbose=verbose, **kwargs, )
Hi
I tried implementing LinearGAM with sklearn's GridsearchCV and got an error when gridsearchCV tried to clone the estimator. The code is below:
gam_rank, gam_cv_results = gam(x_all, y_all)
I get the error
The dataset I used was sklearn's california housing dataset.