chore: improving sklearn compatibility via `parametrize_with_checks` #660

FBruzzesi · 2024-05-09T14:32:35Z

Description

So the story goes as the following:

The CI/CD fails for scikit-learn==1.5rc1 because of a change in the check_estimator internals
In the scikit-learn issue I got a better picture of how to run test for compatible components
In particular, in rolling your own estimator suggests to use parametrize_with_checks, and of course I thought "that is a great idea to avoid dealing manually with each test"
Say no more, I enter a rabbit hole to refactor all our tests - which would be fine
Except that these tests failures helped me figure out a few missing parts in the codebase
Therefore I started to address those missing bits, and that's how you end up with ~~60+~~ 70 files changes

Current status of tests

In estimators:
- DemographicParityClassifier and EqualOpportunityClassifier: n_features_in_ is missing (added those test to the skipping set)
- QuantileRegression(..., solver='L-BFGS-B') fails when some sample_weights are zero.
In meta:
- OrdinalClassifier fails tests because we check that y has at least 3 classes, and I could not find a way to flag that to scikit-learn tests (skipping all tests)
- Thresholder ~~needs some more love~~ added love, now it passes tests.

FBruzzesi · 2024-05-09T14:35:04Z

pyproject.toml

+cvxpy = ["cmake", "osqp", "cvxpy>=1.1.8"]
 formulaic = ["formulaic>=0.6.0"]
-umap = ["umap-learn>=0.4.6", "numpy<2.0"]
+umap = ["umap-learn>=0.4.6"]


That's the good news! They are both working with numpy 2.0rc

FBruzzesi · 2024-05-09T14:36:10Z

sklego/decomposition/pca_reconstruction.py


-    def transform(self, X):


This method was throwing off the checks as they dynamically look for method names

sklego/decomposition/umap_reconstruction.py

FBruzzesi · 2024-05-09T14:37:50Z

sklego/dummy.py

@@ -124,3 +124,6 @@ def allowed_strategies(self):
            DeprecationWarning,
        )
        return self._ALLOWED_STRATEGIES
+
+    def _more_tags(self):
+        return {"poor_score": True, "non_deterministic": True}


I love the poor_score tag 😂😂

FBruzzesi · 2024-05-09T14:38:22Z

sklego/linear_model.py

+        self.n_features_in_ = (
+            X.shape[1] - self.fit_intercept
+        )  # + (1 - int(self.train_sensitive_cols)) * len(self.sensitive_col_idx_)


This is not necessarily true

sklego/meta/ordinal_classification.py

FBruzzesi · 2024-05-09T14:53:20Z

tests/test_estimators/test_lad_regression.py

@@ -80,20 +78,3 @@ def test_fit_intercept_and_copy(coefs, intercept):
 def test_lad(test_fn):
    regr = LADRegression()
    test_fn(LADRegression.__name__, regr)
-
-
-@pytest.mark.parametrize("positive", [True, False])


It's already tested in QuantileRegression, no need to duplicate them

Edit: Actually the entire test can be considered a duplicate

FBruzzesi · 2024-05-09T14:54:14Z

tests/test_estimators/test_quantile_regression.py

@@ -17,8 +18,9 @@

 def _create_dataset(coefs, intercept, noise=0.0):
    np.random.seed(0)
-    X = np.random.randn(10000, coefs.shape[0])


going from 10_000 to 1000 was a big achievement to reduce runtime of the solvers

koaning

All the changes look sensible sofar, but I am wondering if we're perhaps a bit early when it comes to implementing all of this ... the more tags stuff feels a bit experimental. If you don't mind the experimental nature of it then that is fine, but it also feels fine to ignore for now.

sklego/meta/_grouped_utils.py

sklego/meta/ordinal_classification.py

sklego/naive_bayes.py

FBruzzesi · 2024-05-10T08:50:07Z

sklego/linear_model.py

+            return np.average(
+                np.where(X @ params < y, self.quantile, 1 - self.quantile) * np.abs(y - X @ params),
+                weights=sample_weight,
            ) + self._regularized_loss(params)

        def grad_quantile_loss(params):
            return (
                -(sample_weight * np.where(X @ params < y, self.quantile, 1 - self.quantile) * np.sign(y - X @ params))
                @ X
-                / X.shape[0]
+                / sample_weight.sum()


Hey @Garve, sorry to drag you into this mess.

No need for you to read all the changes. TL;DR is that in this PR I added tests using scikit-learn parametrize_with_checks to check for (better) compatibility.

As some of the tests were failing I took a closer look and adjusted the formulas to take into account sample_weight. However we still have tests failing for just one of the solvers. I wonder if you have any clue or insight on why this is the case.

If it's really just one solver I would just not sweat it. We should document it for sure but we can always patch that later.

Apparently, for ubuntu and windows 'TNC' as well is failing, but only for quantile=0.3 and fit_intercept=False, not in the other cases (runs).
I am skipping that as well and improving the warning in the class description

FBruzzesi · 2024-05-10T08:55:13Z

sklego/mixture/bayesian_gmm_detector.py

-        return self.gmm_.score_samples(X) * -1
+        return self.gmm_.score_samples(X)

    def decision_function(self, X):
        # We subtract self.offset_ to make 0 be the threshold value for being an outlier:
-        return self.score_samples(X) + self.likelihood_threshold_
+        return self.score_samples(X) - self.offset_


Both this detector and gmm one required some (breaking changes) to comply to the tests and how scikit expects score_samples, decision_function and predict to interoperate

FBruzzesi · 2024-05-10T08:58:06Z

sklego/meta/thresholder.py

        else:
            try:
-                _ = self.estimator_.predict(X[:1])
+                check_is_fitted(self.estimator_)


From the docs:

If None, estimator is considered fitted if there exist an attribute that ends with a underscore and does not start with double underscore.

I think it's a good proxy. The reason for this is that for non-array data, X is not subscriptable

tests/test_estimators/test_quantile_regression.py

FBruzzesi · 2024-05-15T14:04:42Z

@koaning actually umap is failing on --pre with numpy==2.0rc2
proof

koaning · 2024-05-15T15:35:02Z

Can't we add that numpy req to the umap requirements for the time being? Or does that break something else?

Pragmatically I would assume that umap is an edge case. Most folks probably won't rely on those specific components. The grouped stuff is way more pressing for the library.

FBruzzesi · 2024-05-16T07:12:13Z

@koaning I pinned back numpy<2.0 for the umap extra dependency.
Yesterday I learned that it's not supposed to be compatible yet anyway (comment)

koaning

lgtm!

FBruzzesi added 7 commits May 9, 2024 00:49

big test changes, a lot of red still

174ce16

del file

108b4d1

some more fixes

c7ac899

preprocessing

e936251

grouped and hierarchical

bff99a6

more meta

cd5c19d

meta

293e2e3

FBruzzesi changed the title ~~Tests/sklearn parametrize with checks~~ tests: sklearn parametrize_with_checks May 9, 2024

remove conftest estimator checks

b0b9b1b

FBruzzesi commented May 9, 2024

View reviewed changes

sklego/decomposition/umap_reconstruction.py Outdated Show resolved Hide resolved

FBruzzesi commented May 9, 2024

View reviewed changes

sklego/meta/ordinal_classification.py Show resolved Hide resolved

MRO

af2ba77

FBruzzesi commented May 9, 2024

View reviewed changes

koaning reviewed May 9, 2024

View reviewed changes

sklego/meta/_grouped_utils.py Outdated Show resolved Hide resolved

sklego/meta/ordinal_classification.py Show resolved Hide resolved

sklego/naive_bayes.py Show resolved Hide resolved

FBruzzesi added 4 commits May 9, 2024 17:42

feedback

cc84cf4

fairness estimators

429b950

mixture detector api

1c9681f

all meta components fix

62030fb

FBruzzesi marked this pull request as ready for review May 10, 2024 08:41

FBruzzesi changed the title ~~tests: sklearn parametrize_with_checks~~ chore: improving sklearn compatibility via parametrize_with_checks May 10, 2024

FBruzzesi commented May 10, 2024

View reviewed changes

FBruzzesi and others added 2 commits May 15, 2024 06:24

Merge branch 'koaning:main' into tests/sklearn-parametrize-with-checks

f3a5c1c

skip zero sample weight tests

f09e69b

skip unless SLSQP

230eaf6

FBruzzesi commented May 15, 2024

View reviewed changes

tests/test_estimators/test_quantile_regression.py Outdated Show resolved Hide resolved

specific skips

4addc9e

FBruzzesi requested a review from koaning May 15, 2024 13:51

umap with numpy<2

70b602b

koaning approved these changes May 20, 2024

View reviewed changes

FBruzzesi merged commit 2ae8425 into koaning:main May 20, 2024
17 checks passed

FBruzzesi deleted the tests/sklearn-parametrize-with-checks branch May 20, 2024 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: improving sklearn compatibility via `parametrize_with_checks` #660

chore: improving sklearn compatibility via `parametrize_with_checks` #660

FBruzzesi commented May 9, 2024 •

edited

Loading

FBruzzesi May 9, 2024

FBruzzesi May 9, 2024 •

edited

Loading

FBruzzesi May 9, 2024

FBruzzesi May 9, 2024

FBruzzesi May 9, 2024

FBruzzesi May 15, 2024

FBruzzesi May 9, 2024

koaning left a comment

FBruzzesi May 10, 2024 •

edited

Loading

koaning May 14, 2024

FBruzzesi May 15, 2024 •

edited

Loading

FBruzzesi May 10, 2024

FBruzzesi May 10, 2024

FBruzzesi commented May 15, 2024

koaning commented May 15, 2024 •

edited

Loading

FBruzzesi commented May 16, 2024

koaning left a comment

chore: improving sklearn compatibility via parametrize_with_checks #660

chore: improving sklearn compatibility via parametrize_with_checks #660

Conversation

FBruzzesi commented May 9, 2024 • edited Loading

Description

Current status of tests

Choose a reason for hiding this comment

FBruzzesi May 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koaning left a comment

Choose a reason for hiding this comment

FBruzzesi May 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FBruzzesi May 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FBruzzesi commented May 15, 2024

koaning commented May 15, 2024 • edited Loading

FBruzzesi commented May 16, 2024

koaning left a comment

Choose a reason for hiding this comment

chore: improving sklearn compatibility via `parametrize_with_checks` #660

chore: improving sklearn compatibility via `parametrize_with_checks` #660

FBruzzesi commented May 9, 2024 •

edited

Loading

FBruzzesi May 9, 2024 •

edited

Loading

FBruzzesi May 10, 2024 •

edited

Loading

FBruzzesi May 15, 2024 •

edited

Loading

koaning commented May 15, 2024 •

edited

Loading