Fix search order changing due to using a set in DefaultAlgorithm
#3704
Conversation
| @@ -182,9 +182,11 @@ def _naive_estimators(self): | |||
| return estimators | |||
|
|
|||
| def _non_naive_estimators(self): | |||
| return list( | |||
| set(get_estimators(self.problem_type)) - set(self._naive_estimators()), | |||
There was a problem hiding this comment.
Using a set could change the order. This was only seen recently and between branches as well 😮
There was a problem hiding this comment.
Can we add a test to make sure this doesn't somehow happen again?
There was a problem hiding this comment.
not sure how I can test this since it didn't appear to change within the same run. Any suggestions?
There was a problem hiding this comment.
Hm, not exactly sure. We could hard code the expected order, although we'd have to keep it up to date with every new added estimator. Up to you whether you think we should add something like that or not. Either way, the change looks good!
Codecov Report
@@ Coverage Diff @@
## main #3704 +/- ##
=====================================
Coverage 99.7% 99.7%
=====================================
Files 339 339
Lines 34431 34431
=====================================
Hits 34304 34304
Misses 127 127
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
jeremyliweishih
requested review from
eccabay,
chukarsten,
christopherbunn and
fjlanasa
| @@ -182,9 +182,11 @@ def _naive_estimators(self): | |||
| return estimators | |||
|
|
|||
| def _non_naive_estimators(self): | |||
| return list( | |||
| set(get_estimators(self.problem_type)) - set(self._naive_estimators()), | |||
There was a problem hiding this comment.
Hm, not exactly sure. We could hard code the expected order, although we'd have to keep it up to date with every new added estimator. Up to you whether you think we should add something like that or not. Either way, the change looks good!
Fixes search order changing.