Make HighVarianceCVDataCheck a standalone object/method instead of a data check#1928
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1928 +/- ##
=========================================
- Coverage 100.0% 100.0% -0.0%
=========================================
Files 267 265 -2
Lines 21730 21712 -18
=========================================
- Hits 21724 21706 -18
Misses 6 6
Continue to review full report at Codecov.
|
angela97lin
requested review from
ParthivNaresh,
bchen1116,
chukarsten,
dsherry,
freddyaboulton and
jeremyliweishih
freddyaboulton
left a comment
freddyaboulton
left a comment
There was a problem hiding this comment.
@angela97lin Looks good to me!
| * Updated ``add_results`` in ``AutoMLAlgorithm`` to take in entire pipeline results dictionary from ``AutoMLSearch`` :pr:`1891` | ||
| * Updated ``ClassImbalanceDataCheck`` to look for severe class imbalance scenarios :pr:`1905` | ||
| * Deleted the ``explain_prediction`` function :pr:`1915` | ||
| * Removed ``HighVarianceCVDataCheck`` and convered it to a standalone method instead :pr:`1928` |
There was a problem hiding this comment.
I think this should also be marked as a breaking change
| assert str(warn[0].message).startswith("Argument 'random_state' has been deprecated in favor of 'random_seed'") | ||
|
|
||
|
|
||
| def test_automl_check_for_high_variance(X_y_binary, dummy_binary_pipeline_class, caplog): |
There was a problem hiding this comment.
Not entirely in the scope of this issue but I just noticed we don't test if the high variance warning runs during search. I think it would be worth doing that eventually but we can tackle that in a separate issue. What do you think?
There was a problem hiding this comment.
Yup, you're right--we test that the output is bool but not whether we actually log. I'll add a test 🥳
| high_variance_cv = False | ||
|
|
||
| if cv_scores.mean() != 0: | ||
| high_variance_cv = bool(abs(cv_scores.std() / cv_scores.mean()) > threshold) |
There was a problem hiding this comment.
Isn't abs(cv_scores.std() / cv_scores.mean()) > threshold already a bool?
There was a problem hiding this comment.
@freddyaboulton Unfortunately it resolves to a numpy.bool_ which is different 😭
There was a problem hiding this comment.
Damn. But then how does the old code work? 🤔
There was a problem hiding this comment.
Old code (and this code) check if high_variance_cv which will evaluate to True regardless of python or numpy bool, but now we want to take high_variance_cv and set it in pipeline_results. (Previously, just checked if # of warnings greater than 0, then set high_variance_cv to True so not using it directly.)
I wasn't aware of the difference myself until test_search_results broke at assert isinstance(results['high_variance_cv'], bool) without this type conversion 😬
| pipeline_name = pipeline.name | ||
| high_variance_cv = False | ||
|
|
||
| if cv_scores.mean() != 0: |
There was a problem hiding this comment.
What case is this capturing?
There was a problem hiding this comment.
Hm, this was more a rework of what was already there and the fear of cv_scores.mean() == 0 meaning you divide by 0 in the lines that follow... but it seems like numpy just handles division by 0 as nan, and nan won't ever be greater than the threshold so maybe we don't need this after all. I'll remove!
bchen1116
left a comment
bchen1116
left a comment
There was a problem hiding this comment.
Looks good! I left a few comments on testing/thresholding, but nothing blocking.
| * Updated ``add_results`` in ``AutoMLAlgorithm`` to take in entire pipeline results dictionary from ``AutoMLSearch`` :pr:`1891` | ||
| * Updated ``ClassImbalanceDataCheck`` to look for severe class imbalance scenarios :pr:`1905` | ||
| * Deleted the ``explain_prediction`` function :pr:`1915` | ||
| * Removed ``HighVarianceCVDataCheck`` and convered it to an ``AutoMLSearch`` method instead :pr:`1928` |
| self.add_result_callback(self._results['pipeline_results'][pipeline_id], pipeline, self) | ||
| return pipeline_id | ||
|
|
||
| def _check_for_high_variance(self, pipeline, cv_scores, threshold=0.2): |
There was a problem hiding this comment.
nitpick, but should we enforce that users can't pass in threshold < 0, even though this is a private method?
| assert not automl._check_for_high_variance(pipeline, cv_scores) | ||
|
|
||
| cv_scores = pd.Series([0, 0, 0]) | ||
| assert not automl._check_for_high_variance(pipeline, cv_scores) |
There was a problem hiding this comment.
Might be nice to test different thresholds just to be super comprehensive
5 participants
Closes #1861