-
Notifications
You must be signed in to change notification settings - Fork 116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: improving sklearn compatibility via parametrize_with_checks
#660
chore: improving sklearn compatibility via parametrize_with_checks
#660
Conversation
parametrize_with_checks
pyproject.toml
Outdated
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"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the good news! They are both working with numpy 2.0rc
|
||
def transform(self, X): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method was throwing off the checks as they dynamically look for method names
@@ -124,3 +124,6 @@ def allowed_strategies(self): | |||
DeprecationWarning, | |||
) | |||
return self._ALLOWED_STRATEGIES | |||
|
|||
def _more_tags(self): | |||
return {"poor_score": True, "non_deterministic": True} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love the poor_score
tag 😂😂
sklego/linear_model.py
Outdated
self.n_features_in_ = ( | ||
X.shape[1] - self.fit_intercept | ||
) # + (1 - int(self.train_sensitive_cols)) * len(self.sensitive_col_idx_) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not necessarily true
@@ -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]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already tested in QuantileRegression
, no need to duplicate them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit: Actually the entire test can be considered a duplicate
@@ -17,8 +18,9 @@ | |||
|
|||
def _create_dataset(coefs, intercept, noise=0.0): | |||
np.random.seed(0) | |||
X = np.random.randn(10000, coefs.shape[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
going from 10_000
to 1000
was a big achievement to reduce runtime of the solvers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
parametrize_with_checks
parametrize_with_checks
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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
else: | ||
try: | ||
_ = self.estimator_.predict(X[:1]) | ||
check_is_fitted(self.estimator_) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Description
So the story goes as the following:
check_estimator
internalsparametrize_with_checks
, and of course I thought "that is a great idea to avoid dealing manually with each test"60+70 files changesCurrent status of tests
estimators
:DemographicParityClassifier
andEqualOpportunityClassifier
:n_features_in_
is missing (added those test to the skipping set)QuantileRegression(..., solver='L-BFGS-B')
fails when some sample_weights are zero.meta
:OrdinalClassifier
fails tests because we check thaty
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 loveadded love, now it passes tests.