-
Notifications
You must be signed in to change notification settings - Fork 87
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
Update AutoSearchBase
to use dynamically generated preprocessing pipelines
#870
Conversation
Codecov Report
@@ Coverage Diff @@
## master #870 +/- ##
========================================
Coverage 99.76% 99.77%
========================================
Files 193 193
Lines 8603 8830 +227
========================================
+ Hits 8583 8810 +227
Misses 20 20
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.
i tested on a local dataset and it work and did a quick pass through the code and left some comments. has my approval, but id get one more.
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 I didn't review the tests yet, will do. But I left comments on the impl. Looks good! Mostly minor stuff. Ping me whenever you feel this is ready for re-review and I'll read the whole thing.
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 good! Left suggestions on splitting up the tests, parametrize, cleaning up dead code in the impl and logging. I'd say the only blocking thing is deleting the dead code, but would be great to get the other stuff in too, either in this PR or separate
@dsherry FYI I split the tests up as you suggested, and parameterized some but not all, my reason being that it got really unwieldy to add in all of the different parameters (dataset, mocks, mock pipelines) to support binary/multiclass and this seemed cleaner. We can always revisit though! |
Closes #844
Notes:
self.allowed_pipelines
isn't instantiated untilsearch()
if not supplied as a parameter, is that weird? (same withself.allowed_model_families
)