-
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
Remove calculating plot metrics from AutoML #615
Conversation
Codecov Report
@@ Coverage Diff @@
## improved_objectives #615 +/- ##
=======================================================
+ Coverage 98.81% 98.89% +0.08%
=======================================================
Files 130 131 +1
Lines 4713 4452 -261
=======================================================
- Hits 4657 4403 -254
+ Misses 56 49 -7
Continue to review full report at Codecov.
|
fpr, tpr, thresholds = roc_metric.score(y_predict_proba, y_true) | ||
assert not np.isnan(fpr).any() | ||
assert not np.isnan(tpr).any() | ||
assert not np.isnan(thresholds).any() |
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.
Ah, this was to satisfy codecov?
There's a lot of negative testing we should do here: try passing in some nans
/infs
, all 0s/all 1s, probabilities outside the range 0 to 1, etc. To keep things moving, rather than doing it now, let's just include that in the issue we have to file to add ROC/confusion back in once this is on master.
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.
Yup, because we removed calls to calculating confusion matrix and ROC so I added this test to satisfy codecov. I guess we had this talk about adding accuracy as well, but I think we should better scope out what exactly what type of test coverage we want to provide for these metrics (NaNs, inf, etc) and update that for all of our metrics then?
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.
Yeah, for sure. We could even have a test which tries certain input on all classification objectives and other input on all regression objectives, and then does so with varieties of nans and infs and stuff.
However, ROC and classification aren't objectives, they're plot data util methods, so they don't need to get identical treatment, and shouldn't be grouped in with the objectives.
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.
True, the way I was thinking is so pre-improved-objectives :P
@angela97lin you missed one spot in the docs. From readthedocs:
We gotta find a way to have warnings fail the docs checkin test! (#586) |
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! Once you fix the readthedocs warnings I just commented about, good to merge.
Please file an issue to add ROC and confusion matrix back in, so we can do that next week.
Here's how long the unit tests took to run on linux python 3.8 for each relevant branch:
master
: 3m57s
improved_objectives
: 7m22s 💣
this branch: 4m40s
Muuuch better! 😄
* Objectives API: Create new binary / multiclass pipeline classes and remove objectives from pipeline classes (#405) * Objectives API: Remove ROC and confusion matrix as objectives (#422) * Change `score` output to return one dictionary (#429) * Create binary and multiclass objective classes (#504) * Update dependencies (#412) * Hide features with zero importance in plot by default (#413) * Update dependencies check: package whitelist (#417) * Add fixes necessary for docs to build for improved objectives project (#605) * Remove calculating plot metrics from AutoML (#615)
As part of resolving #607, removing calculating plot metrics from AutoML.
Note that this will currently break all calls to plotting methods!