-
Notifications
You must be signed in to change notification settings - Fork 86
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
Adds util functions needed for dynamic prepocessing pipelines #852
Conversation
Codecov Report
@@ Coverage Diff @@
## master #852 +/- ##
========================================
Coverage 99.70% 99.70%
========================================
Files 195 195
Lines 8000 8181 +181
========================================
+ Hits 7976 8157 +181
Misses 24 24
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is exciting! 🎊 great stuff. Left some suggestions and docs tweaks, but nothing blocking merge IMO!
We should resolve the convo about adding problem_type
to get_preprocessing_pipeline
sometime soon though (before the release).
Ideas for more unit test coverage:
- Check input and output dtypes--does
make_pipeline
work withnp.array
as input?pd.DataFrame
with no column names? - Validate that we throw if the provided estimator isn't an
Estimator
subclass -- I think you already have this
That's all I could think of right now!
Food for thought for the future: we will likely end up supporting different versions of the preprocessing functions. Some may even be in external modules. We don't need to worry about this in this PR, but we should consider: what could we do to make backwards compatibility easy? One idea: add a "preprocessing_version" string input, which we could use to map to the impl. Another idea: keeping get_preprocessing_pipeline
private could help us have more flexibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks solid to me
I agree with @dsherry here. we can make this public later if we chooce. especially since we have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@angela97lin thanks for re-requesting. I left a comment in response on all_estimators
. Once that's resolved LGTM
…ml into 843_preprocessing_utils
Closes #843
Q's: