-
Notifications
You must be signed in to change notification settings - Fork 90
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
MAINT activate common test sklearn #647
Conversation
There is a failing test. It seems that the TableVectorizer no longer accepts 1D inputs. This is a good thing, but the changelog must be updated to reflect this, and the test updated. |
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.
One minor comment and then merge
for j in range(X.shape[1]): | ||
X[i, j] = str(X[i, j]) | ||
elif _safe_tags(estimator, key="allow_nan"): | ||
X = X.astype(np.float64) |
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.
Why float64 and not 32?
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.
The scikit-learn conversion will be np.float64
so it will be better for all common tests.
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.
👍
|
||
def test_sklearn_compatible_GapEncoder(): | ||
check_estimator(GapEncoder()) | ||
if estimator.__class__.__name__ == "SkewedChi2Sampler": |
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.
Aaaaaaaah 🤣 🤣 🤣 🤣
|
||
|
||
def test_sklearn_compatible_SimilarityEncoder(): | ||
check_estimator(SimilarityEncoder()) |
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.
Please add a todo here
You have another test failing. The feature names have changed (I'm a bit surprised, I don't see where the change comes from, but it does not matter terribly). I don't mind the change of name. We should however update the test and maybe mention it in the changelog. |
The reason is that we now first |
However, this is a regression because this is now different from what the scikit-learn encoder are doing. |
Right, that might be a problem |
LGTM. Updating the branch with main and then merging. |
Run scikit-learn common test.