Repository: scikit-learn/scikit-learn
File: sklearn/discriminant_analysis.py (Python)

Code before:
                    "Try solver='eigen' instead."
                )

        means = []
        cov = []
        scalings = []
        rotations = []
        for idx, group in enumerate(self.classes_):
            X_class = X[y == group, :]
            if len(X_class) == 1:
                raise ValueError(
                    "y has only 1 sample in class %s, covariance is ill defined."
                    % str(self.classes_[idx])
                )

            mean_class = X_class.mean(0)
            means.append(mean_class)

            if self.solver == "svd":
                scaling_class, rotation_class, cov_class = self._solve_svd(X_class)
            elif self.solver == "eigen":
                scaling_class, rotation_class, cov_class = self._solve_eigen(
                    X_class,
                    shrinkage=self.shrinkage,
                    covariance_estimator=self.covariance_estimator,
                )

            rank = np.sum(scaling_class > self.tol)
            if rank < n_features:
                msg_param = "shrinkage" if self.solver == "eigen" else "reg_param"
                warnings.warn(
                    f"The covariance matrix of class {group} is not full rank. "
                    "Increasing the value of parameter `{msg_param}` might help"
                    " reducing the collinearity.",
                    linalg.LinAlgWarning,
                )

            cov.append(cov_class)
            scalings.append(scaling_class)
            rotations.append(rotation_class)

        if self.store_covariance:
            self.covariance_ = cov
        self.means_ = np.asarray(means)
        self.scalings_ = scalings
        self.rotations_ = rotations
        return self

    def _decision_function(self, X):
        # return log posterior, see eq (4.12) p. 110 of the ESL.
        check_is_fitted(self)

Review comment (suggestion):
Just another small suggestion from an outside observer, you can put this whole if statement outside the loop, something like

if self.solver == "svd":
  specific_solver = self._solve_svd
elif self.solver = "eigen":
  specific_solver = self._solve_eigen

and then in the loop just call specific_solver(X_class)

It also already fits in with the existing if statement above, namely if solver is "svd", then validate the parameters, and if they are fine set the solver method. I think it will make the loop a bit cleaner. The only problem is that solve_eigen requires extra parameters, but you can use either lambda function, or if for now it always uses the same parameters, just remove them as parameters to the method.


Code after:
        scalings = []
        rotations = []
        for class_idx, class_label in enumerate(self.classes_):
            X_class = X[y == class_label, :]
            if len(X_class) == 1:
                raise ValueError(
                    "y has only 1 sample in class %s, covariance is ill defined."
                    % str(self.classes_[class_idx])
                )

            mean_class = X_class.mean(0)
            means.append(mean_class)

            scaling_class, rotation_class, cov_class = specific_solver(X_class)

            rank = np.sum(scaling_class > self.tol)
            if rank < n_features:
                n_samples_class = X_class.shape[0]
                if self.solver == "svd" and n_samples_class <= n_features:
                    raise linalg.LinAlgError(
                        f"The covariance matrix of class {class_label} is not full "
                        f"rank. When using `solver='svd'` the number of samples in "
                        f"each class should be more than the number of features, but "
                        f"class {class_label} has {n_samples_class} samples and "
                        f"{n_features} features. Try using `solver='eigen'` and "
                        f"setting the parameter `shrinkage` for regularization."
                    )
                else:
                    msg_param = "shrinkage" if self.solver == "eigen" else "reg_param"
                    raise linalg.LinAlgError(
                        f"The covariance matrix of class {class_label} is not full "
                        f"rank. Increase the value of `{msg_param}` to reduce the "
                        f"collinearity.",
                    )

            cov.append(cov_class)
            scalings.append(scaling_class)
            rotations.append(rotation_class)

        if self.store_covariance:
            self.covariance_ = cov
        self.means_ = np.asarray(means)
        self.scalings_ = scalings
        self.rotations_ = rotations
        return self

    def _decision_function(self, X):
        # return log posterior, see eq (4.12) p. 110 of the ESL.
        check_is_fitted(self)

        X = validate_data(self, X, reset=False)