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

[ENH] remove fit_params kwargs throughout the code base #2343

Merged
merged 7 commits into from
Mar 29, 2022
Merged

Conversation

fkiraly
Copy link
Collaborator

@fkiraly fkiraly commented Mar 29, 2022

As pointed out by @Vasudeva-bit, some estimators were still using a fit_params kwarg in _fit.

This is non-compliant with the interface spec, but did not cause any errors since never anything was passed by kwargs.

This PR simply removes any fit_params kwargs and concomitant logic from the code base.
Needs no deprecation, since:

  • the interface spec is clear and the args occurred only in private functions, with one exception.
  • evaluate had the arg, but when used it would error out since no fit actually had the arg, so deprecation is also not necessary here (because we are changing from an "error causing argument" to "no argument", hence no one will have relied on it).

Since this seems to be a frequent issue for extenders, I've added a paragraph to _fit in the classification, forecasting, and transformer extension templates (where it is most likely encountered due to interfacing as opposed to implementation from scratch).

@fkiraly fkiraly added the refactor Restructuring without changing its external behavior. Neither fixing a bug nor adding a feature. label Mar 29, 2022
Copy link
Contributor

@aiwalter aiwalter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you check what those params could have been? We might add them to constructor as we did for statsmodels in another PR

@fkiraly
Copy link
Collaborator Author

fkiraly commented Mar 29, 2022

Did you check what those params could have been?

As far as I saw, only occurred in compositors, and logic passing the kwargs from composite interfaces to component interfaces.

Plus pmdarima, I'll open an issue for it: #2344

@aiwalter
Copy link
Contributor

I checked for Prophet, and yes that **kwargs in _fit() were actually not reachable due to inner call in fit()

@fkiraly
Copy link
Collaborator Author

fkiraly commented Mar 29, 2022

@aiwalter, are there any Prophet args which we are missing due to this?

@fkiraly
Copy link
Collaborator Author

fkiraly commented Mar 29, 2022

Added prophet to the issue #2344

fkiraly and others added 2 commits March 29, 2022 20:54
aiwalter
aiwalter previously approved these changes Mar 29, 2022
@fkiraly
Copy link
Collaborator Author

fkiraly commented Mar 29, 2022

@aiwalter, this has now come up so often, I've added a paragraph to extension templates in _fit - do you think that's a good idea?

@aiwalter
Copy link
Contributor

yes, seems many dependency packages are doing this, so we have to add it to constructor. Makes sense to add it to template. Thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Restructuring without changing its external behavior. Neither fixing a bug nor adding a feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants