[MRG] ENH: process DataFrames in OneHot/OrdinalEncoder without converting to array #12147 #13253
Conversation
…into data_frames
…into data_frames
sklearn/preprocessing/_encoders.py
Outdated
| @@ -41,6 +41,9 @@ def _check_X(self, X): | |||
| not do that) | |||
|
|
|||
| """ | |||
| if hasattr(X, 'iloc'): | |||
| # if pandas dataframes | |||
There was a problem hiding this comment.
Can you add some comment here about why we skip the check for pandas? (because it will be done column by column later on).
Or maybe add it to the docstring above.
There was a problem hiding this comment.
I added a comment (as above)
and check if instead of pandas DataFrame Sequence is not passed
and a couple of tests
…into data_frames
…into data_frames
…st for test_X_is_not_1D_pandas
…ture by feature; validation is done on the very beginning for all data types this way.
…into data_frames
|
@jnothman can you give this a new look? It's now with the list of arrays idea (I am not yet fully sure it is better, but it works) |
|
Yes, let's not be too picky if X is a list... It is either interpreted as
numeric or as objects
|
sklearn/preprocessing/_encoders.py
Outdated
| if not (hasattr(X, 'iloc') and getattr(X, 'ndim', 0) == 2): | ||
| X_temp = check_array(X, dtype=None) | ||
| if not hasattr(X, 'dtype') and\ | ||
| np.issubdtype(X_temp.dtype, np.str_): |
There was a problem hiding this comment.
can you format this as:
if (not hasattr(X, 'dtype')
and np.issubdtype(X_temp.dtype, np.str_)):
(avoiding the \)
sklearn/preprocessing/_encoders.py
Outdated
| if hasattr(X, 'iloc'): | ||
| # pandas dataframes | ||
| return X.iloc[:, key] | ||
| else: |
There was a problem hiding this comment.
you can remove else and unindent the return statement below.
| return X_columns, n_samples, n_features | ||
|
|
||
| def _get_feature(self, X, key): | ||
| if hasattr(X, 'iloc'): |
There was a problem hiding this comment.
return X.iloc[:, key] if hasattr(X, 'iloc) else X[:, key]There was a problem hiding this comment.
It's certainly less lines, but not sure if it improves readability :-)
There was a problem hiding this comment.
Skip it if you think it is best. I don't have strong opinion here.
sklearn/preprocessing/_encoders.py
Outdated
|
|
||
| return X | ||
| for i in range(n_features): | ||
| Xi = self._get_feature(X, key=i) |
There was a problem hiding this comment.
Is there a reason to call it key and not feature_idx or feature_index?
| @pytest.mark.parametrize("X", [ | ||
| [1, 2], | ||
| np.array([3., 4.]) | ||
| ]) |
There was a problem hiding this comment.
@pytest.mark.parametrize(""method", ['fit', 'fit_transform'])
| [1, 2], | ||
| np.array([3., 4.]) | ||
| ]) | ||
| def test_X_is_not_1D(X): |
There was a problem hiding this comment.
| def test_X_is_not_1D(X): | |
| def test_X_is_not_1D(X, method): |
|
|
||
| msg = ("Expected 2D array, got 1D array instead") | ||
| with pytest.raises(ValueError, match=msg): | ||
| oh.fit(X) |
There was a problem hiding this comment.
| oh.fit(X) | |
| getattr(oh, method)(X) |
| msg = ("Expected 2D array, got 1D array instead") | ||
| with pytest.raises(ValueError, match=msg): | ||
| oh.fit(X) | ||
| with pytest.raises(ValueError, match=msg): |
There was a problem hiding this comment.
Then you can remove this part
| oh.fit_transform(X) | ||
|
|
||
|
|
||
| def test_X_is_not_1D_pandas(): |
There was a problem hiding this comment.
I would do the same parametrization than above.
Co-Authored-By: maikia <maja_ka@hotmail.com>
Co-Authored-By: maikia <maja_ka@hotmail.com>
…into data_frames
|
Please resolve conflicts with master, @maikia, then we can merge! Thanks. |
…into data_frames
|
Thanks! |
|
Thanks :-) |
…erting to array scikit-learn#12147 (scikit-learn#13253)" This reverts commit d94af6f.
…erting to array scikit-learn#12147 (scikit-learn#13253)" This reverts commit d94af6f.
Issue: ENH: support DataFrames in OneHot/OrdinalEncoder without converting to array #12147
if features are given as pandas dframe, each column keeps its datatype
Reference Issues/PRs
What does this implement/fix? Explain your changes.
Any other comments?
Closes #12147