[MRG+1] FIX bug where ransac is running too many iterations (scikit-learn#8271)

Author: aivision2020

doc/whats_new.rst

@@ -195,6 +195,9 @@ Bug fixes
      sparse input.
      :issue:`8259` by :user:`Aman Dalmia <dalmia>`.
 
+   - Fixed a bug where :func:`sklearn.linear_model.RANSACRegressor.fit` may run until
+     ``max_iter`` if finds a large inlier group early. :issue:`8251` by :user:`aivision2020`.
+
    - Fixed a bug where :func:`sklearn.datasets.make_moons` gives an
      incorrect result when ``n_samples`` is odd.
      :issue:`8198` by :user:`Josh Levy <levy5674>`.

sklearn/linear_model/ransac.py

@@ -342,7 +342,10 @@ def fit(self, X, y, sample_weight=None):
 
         n_samples, _ = X.shape
 
-        for self.n_trials_ in range(1, self.max_trials + 1):
+        self.n_trials_ = 0
+        max_trials = self.max_trials
+        while self.n_trials_ < max_trials:
+            self.n_trials_ += 1
 
             if (self.n_skips_no_inliers_ + self.n_skips_invalid_data_ +
                     self.n_skips_invalid_model_) > self.max_skips:
@@ -416,13 +419,14 @@ def fit(self, X, y, sample_weight=None):
             X_inlier_best = X_inlier_subset
             y_inlier_best = y_inlier_subset
 
+            max_trials = min(
+                max_trials,
+                _dynamic_max_trials(n_inliers_best, n_samples,
+                                    min_samples, self.stop_probability))
+
             # break if sufficient number of inliers or score is reached
-            if (n_inliers_best >= self.stop_n_inliers
-                    or score_best >= self.stop_score
-                    or self.n_trials_
-                       >= _dynamic_max_trials(n_inliers_best, n_samples,
-                                              min_samples,
-                                              self.stop_probability)):
+            if n_inliers_best >= self.stop_n_inliers or \
+                            score_best >= self.stop_score:
                 break
 
         # if none of the iterations met the required criteria

sklearn/linear_model/tests/test_ransac.py

@@ -22,10 +22,9 @@
 data = np.column_stack([X, y])
 
 # Add some faulty data
-outliers = np.array((10, 30, 200))
-data[outliers[0], :] = (1000, 1000)
-data[outliers[1], :] = (-1000, -1000)
-data[outliers[2], :] = (-100, -50)
+rng = np.random.RandomState(1000)
+outliers = np.unique(rng.randint(len(X), size=200))
+data[outliers, :] += 50 + rng.rand(len(outliers), 2) * 10
 
 X = data[:, 0][:, np.newaxis]
 y = data[:, 1]
@@ -90,13 +89,16 @@ def test_ransac_max_trials():
                                        random_state=0)
     assert_raises(ValueError, ransac_estimator.fit, X, y)
 
-    ransac_estimator = RANSACRegressor(base_estimator, min_samples=2,
-                                       residual_threshold=5, max_trials=11,
-                                       random_state=0)
-    assert getattr(ransac_estimator, 'n_trials_', None) is None
-    ransac_estimator.fit(X, y)
-    assert_equal(ransac_estimator.n_trials_, 2)
-
+    # there is a 1e-9 chance it will take these many trials. No good reason
+    # 1e-2 isn't enough, can still happen
+    # 2 is the what ransac defines  as min_samples = X.shape[1] + 1
+    max_trials = _dynamic_max_trials(
+        len(X) - len(outliers), X.shape[0], 2, 1 - 1e-9)
+    ransac_estimator = RANSACRegressor(base_estimator, min_samples=2)
+    for i in range(50):
+        ransac_estimator.set_params(min_samples=2, random_state=i)
+        ransac_estimator.fit(X, y)
+        assert_less(ransac_estimator.n_trials_, max_trials + 1)
 
 def test_ransac_stop_n_inliers():
     base_estimator = LinearRegression()
@@ -383,6 +385,7 @@ def test_ransac_residual_metric():
     assert_array_almost_equal(ransac_estimator0.predict(X),
                               ransac_estimator2.predict(X))
 
+
 def test_ransac_residual_loss():
     loss_multi1 = lambda y_true, y_pred: np.sum(np.abs(y_true - y_pred), axis=1)
     loss_multi2 = lambda y_true, y_pred: np.sum((y_true - y_pred) ** 2, axis=1)