Skip to content

Conversation

@qiagu
Copy link
Contributor

@qiagu qiagu commented Mar 26, 2019

Description

Current named_regressors and the fitting regressors are disconnected. When replacing the one of regressors, either base or meta, in GridSearchCV, the functioning regressors remain unchanged.
The nice naming of the meta_regressor is lost after this PR, but it seems sklearn processes many single estimator parameters in such way.
After this PR, all keys in the get_params are able to set_params and functional in SearchCV.

Related issues or pull requests

Pull Request Checklist

  • Added a note about the modification or contribution to the ./docs/sources/CHANGELOG.md file (if applicable)
  • Added appropriate unit test functions in the ./mlxtend/*/tests directories (if applicable)
  • Modify documentation in the corresponding Jupyter Notebook under mlxtend/docs/sources/ (if applicable)
  • Ran nosetests ./mlxtend -sv and make sure that all unit tests pass (for small modifications, it might be sufficient to only run the specific test file, e.g., nosetests ./mlxtend/classifier/tests/test_stacking_cv_classifier.py -sv)
  • Checked for style issues by running flake8 ./mlxtend

@qiagu
Copy link
Contributor Author

qiagu commented Mar 26, 2019

@rasbt If you like this PR, I can add a test for the regressor replacement.

@coveralls
Copy link

coveralls commented Mar 26, 2019

Coverage Status

Coverage increased (+0.03%) to 91.543% when pulling cf2db67 on qiagu:stackingcv into 3f0935a on rasbt:master.

@rasbt
Copy link
Owner

rasbt commented Mar 26, 2019

Thanks a lot, I really like this PR. I think the naming change is in the worst case a bit unfortunate, but not a big deal, because it's a substantial improvement overall.

I can add a test for the regressor replacement.

that would be great!

Also, we may have to clean up the documentation a little bit (the documentation files are in https://github.com/rasbt/mlxtend/tree/master/docs/sources/user_guide from which the HTML docs are auto-generated upon version updates)

I think this will allow us to have much better compatibility with GridSearchCV like you mentioned -- there were a couple of questions about this in the past. (Probably related to #511)

Also, but this would be a bit of extra work as we haven't finished the refactoring, it would be nice to bring this to the other stacking classes as well (StackingClassifier, StackingCVClassifier, and StackingRegressor).

@qiagu
Copy link
Contributor Author

qiagu commented Mar 26, 2019

Thanks, @rasbt. I'll make a test and update the documentation file, but my work for this PR, probably, will be limited to StackingCVRegressor. I can come back later for other stacking estimators after I finish current tasks on hand, if those estimators still need love at that time.
For SequentialFeatureSelector in #511, I see that GridSearchCV is able to change the attributes estimator and named_est, but not est_, whereas the last one carries the real estimator doing the fit and calculating scores. So, yes, they are similar issues.

@rasbt
Copy link
Owner

rasbt commented Mar 27, 2019

probably, will be limited to StackingCVRegressor. I can come back later for other stacking estimators after I finish current tasks on hand, if those estimators still need love at that time.

No worries, Rome wasn't built in one day, I will add it to the "Issues" to do list :)

in #511, I see that GridSearchCV is able to change the attributes estimator and named_est, but not est_, whereas the last one carries the real estimator doing the fit and calculating scores. So, yes, they are similar issues.

Good catch, yeah, I think there are lot of these things where using more of scikit-learn's paradigms would be useful to mitigate these issues.

@qiagu
Copy link
Contributor Author

qiagu commented Mar 27, 2019

@rasbt Finally, I make a base utility which could be applied to other stacking estimators. Also, ordered parameter replacement seems to be possible now.
I often feel struggling in English and naming stuff, please feel free to correct my doc, comment, naming and other things.
If there is still a missing piece for StackingCVRegressor, please let me know.

@rasbt
Copy link
Owner

rasbt commented Mar 28, 2019

Thanks a lot, this is great! Only made a few minor tweaks and looking forward to merging once the tests pass.

@qiagu
Copy link
Contributor Author

qiagu commented Mar 28, 2019

haha, typo.

@rasbt
Copy link
Owner

rasbt commented Mar 28, 2019

Oh, didn't catch that one, thanks again!

@rasbt rasbt merged commit 6485619 into rasbt:master Mar 28, 2019
@qiagu qiagu deleted the stackingcv branch April 26, 2019 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants