Check X and y index match when encoding, closes #376 #377
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,12 +2,14 @@ | |
| transform(). | ||
| """ | ||
|
|
||
| from typing import List, Tuple, Union | ||
|
|
||
| import numpy as np | ||
| import pandas as pd | ||
| from scipy.sparse import issparse | ||
|
|
||
| from .numpy_to_pandas import _is_numpy, _numpy_to_dataframe, _numpy_to_series | ||
|
|
||
|
|
||
| def _is_dataframe(X: pd.DataFrame) -> pd.DataFrame: | ||
| """ | ||
|
|
@@ -33,9 +35,8 @@ def _is_dataframe(X: pd.DataFrame) -> pd.DataFrame: | |
| """ | ||
| # check_estimator uses numpy arrays for its checks. | ||
| # Thus, we need to allow np arrays | ||
| if _is_numpy(X): | ||
| X = _numpy_to_dataframe(X) | ||
|
|
||
| if issparse(X): | ||
| raise ValueError("This transformer does not support sparse matrices.") | ||
|
|
@@ -129,3 +130,86 @@ def _check_contains_inf(X: pd.DataFrame, variables: List[Union[str, int]]) -> No | |
| "Some of the variables to transform contain inf values. Check and " | ||
| "remove those before using this transformer." | ||
| ) | ||
|
|
||
|
|
||
| def _check_pd_X_y( | ||
| X: Union[pd.DataFrame, np.ndarray], | ||
| y: Union[pd.Series, np.ndarray, list, Tuple], | ||
| ): | ||
| """ | ||
| Returns X as a DataFrame and y as a Series, converting any numpy | ||
| objects to pandas objects as needed. | ||
| * If both parameters are numpy objects, they are converted to pandas objects. | ||
| * If one parameter is a pandas object and the other is a numpy object, | ||
| the former will be converted to a pandas object, with the indexes | ||
| of the latter. | ||
| * If both parameters are pandas objects, and their indexes are inconsistent, | ||
| an exception is raised (i.e. this is the caller's error.) | ||
| * If both parameters are pandas objects and their indexes match, they are | ||
| copied and returned. | ||
| * If X is sparse or X is empty or, after all transforms, is stiil | ||
| not a DataFrame, raises an exception | ||
| * Raises an exception if either incoming object is None or empty | ||
|
|
||
| Parameters | ||
| ---------- | ||
| X: Pandas DataFrame or numpy ndarray | ||
| y: Pandas Series or numpy ndarray or list or tuple | ||
|
|
||
| Returns | ||
| ------- | ||
| X: Pandas DataFrame | ||
| y: Pandas Series | ||
|
|
||
| Exceptions | ||
| ---------- | ||
| ValueError: if X and y are dimension-incompatible, X and y are pandas objects | ||
| with inconsistent indexes, or if either X or y is None/empty | ||
| """ | ||
| # * Raises an exception if either incoming object is None or empty | ||
| if X is None or len(X) == 0: | ||
| raise ValueError("X cannot be None or empty") | ||
| if y is None or len(y) == 0: | ||
| raise ValueError("y cannot be None or empty") | ||
|
|
||
| # * If both parameters are numpy objects, they are converted to pandas objects. | ||
| # * If one parameter is a pandas object and the other is a numpy object, | ||
| # the former will be converted to a pandas object, with the indexes | ||
| # of the latter. (Lists and tuples are also supported for y) | ||
| if _is_numpy(X): | ||
| X = _numpy_to_dataframe(X, index=y.index if isinstance(y, pd.Series) else None) | ||
| if _is_numpy(y): | ||
| y = _numpy_to_series(y, index=X.index if isinstance(X, pd.DataFrame) else None) | ||
| if isinstance(y, (list, Tuple)): | ||
| y = pd.Series(y) | ||
| y.index = X.index if isinstance(X, pd.DataFrame) else None | ||
|
|
||
| # * If both parameters are pandas objects, and their indexes are inconsistent, | ||
| # an exception is raised (i.e. this is the caller's error.) | ||
| # * If both parameters are pandas objects and their indexes match, they are | ||
| # copied and returned | ||
| if isinstance(X, pd.DataFrame) and isinstance(y, pd.Series): | ||
| if not all(y.index == X.index): | ||
| raise ValueError("Index mismatch between DataFrame X and Series y") | ||
| else: | ||
| return X.copy(), y.copy() | ||
|
|
||
| # * If X is sparse or X is empty or, after all transforms, is stiil | ||
| # not a DataFrame, raises an exception | ||
| # (This deliberately carries out similar tests in _is_dataframe() above in | ||
| # order to support different code paths) | ||
| if issparse(X): | ||
| raise ValueError("This transformer does not support sparse matrices.") | ||
|
|
||
| if not isinstance(X, pd.DataFrame): | ||
| raise TypeError( | ||
| "X is not a pandas dataframe. The dataset should be a pandas dataframe." | ||
| ) | ||
|
|
||
| if X.empty: | ||
| raise ValueError( | ||
| "0 feature(s) (shape=%s) while a minimum of %d is " | ||
| "required." % (X.shape, 1) | ||
| ) | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. I think we could simplify this test a bit. X can only be a pandas dataframe at this stage, because if it was an array, So when we call this function within the classes, the only chance is X is a dataframe, and y can be, in theory, an array or a series. I think sklearn also allows arrays as targets, but I don't think we can use that with the encoders. Our encoders are tailored to binary classification mostly. So we probably need that check as well :_(
Collaborator
There was a problem hiding this comment. I think, given the scenarios that I posed in #376 the best solution would be to replace _is_dataframe in the encoders by a new function that checks simultaneously X and y, because depending on the input combination we should raise errors or not. thoughts?
Contributor
Author
There was a problem hiding this comment. I was thinking something like that earlier but was afraid to make changes to calls to
Contributor
Author
There was a problem hiding this comment.
In commits from earlier, I deliberately put the call to
This is correct. I'll remove the logic that handles cases of I do feel good about merging the functionality of I have additional followup on the #376 page.
Collaborator
There was a problem hiding this comment. I would not change is_dataframe because it is used all over our codebase. I would do the following: if the encoder needs X and y, instead of using _is_dataframe, use directly your check and return a pandas dataframe and a pd series. If encoder does not need y: then use is_dataframe as usual. If encoder has the option to do both (OrdinalEncoder): This means, taking the _is_dataframe out of _check_input_and_variables() hidden method in the base_encoder and amending the code slightly in all the classes. |
||
| return X, y | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,7 +53,7 @@ def __init__( | |
| self.variables = _check_input_parameter_variables(variables) | ||
| self.ignore_format = ignore_format | ||
|
|
||
| def _check_fit_input_and_variables(self, X: pd.DataFrame): | ||
| """ | ||
| Checks that input is a dataframe, finds categorical variables, or alternatively | ||
| checks that the variables entered by the user are of type object (categorical). | ||
|
|
@@ -71,18 +71,8 @@ def _check_fit_input_and_variables(self, X: pd.DataFrame) -> pd.DataFrame: | |
| ValueError | ||
| If there are no categorical variables in the df or the df is empty | ||
| If the variable(s) contain null values | ||
| """ | ||
|
|
||
| if not self.ignore_format: | ||
| # find categorical variables or check variables entered by user are object | ||
| self.variables_: List[ | ||
|
|
@@ -101,7 +91,7 @@ def _check_fit_input_and_variables(self, X: pd.DataFrame) -> pd.DataFrame: | |
| # save train set shape | ||
| self.n_features_in_ = X.shape[1] | ||
|
|
||
| return self | ||
|
There was a problem hiding this comment. The typehint and docstring should be changed to match this updated return behavior. I might suggest not returning anything, but I did see @solegalli suggested specifically
Contributor
Author
There was a problem hiding this comment. OK fixed in latest commit. Convention in rest of code for
Collaborator
There was a problem hiding this comment. @bmreiniger your comment was very enlightening. Following sklearn convention, the fit method needs to return self. And I now found out that it might be to allow method cascading, so class.fit().transform() which would not be possible without returning self. So indeed, there is no need to return self in this method, because we won't cascade it. In fact, it is an internal method. Here is the reference I found, mostly for my information lol: Thank you!
Contributor
Author
There was a problem hiding this comment. @solegalli OK should I change to not return |
||
|
|
||
| def _check_transform_input_and_state(self, X: pd.DataFrame) -> pd.DataFrame: | ||
| """ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| """Functions to detect numpy objects and convert to pandas objects.""" | ||
|
|
||
| from typing import Any, List, Union | ||
|
|
||
| import numpy as np | ||
| import pandas as pd | ||
|
|
||
|
|
||
| def _is_numpy(obj_in: Any) -> bool: | ||
| """ | ||
| Tests if an object is a numpy object. | ||
| If the input is a numpy array, it converts it to a pandas Dataframe. This is mostly | ||
| so that we can add the check_estimator checks for compatibility with sklearn. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| obj_in : the object to test. | ||
|
|
||
| Returns | ||
| ------- | ||
| True if object is a numpy object, else False | ||
| """ | ||
| return isinstance(obj_in, (np.generic, np.ndarray)) | ||
|
|
||
|
|
||
| def _numpy_to_dataframe( | ||
| obj_in: Union[np.generic, np.ndarray], index=None | ||
| ) -> pd.DataFrame: | ||
| """ | ||
| Converts a numpy object to a pandas DataFrame | ||
|
|
||
| Parameters | ||
| ---------- | ||
| obj_in : the object to convert | ||
| index : array-like (optional); will set index on DataFrame | ||
|
|
||
| Returns | ||
| ------- | ||
| df_out : the object converted to a pandas DataFrame | ||
| """ | ||
| col_names: List[str] = [str(i) for i in range(obj_in.shape[1])] | ||
| df_out: pd.DataFrame = pd.DataFrame(obj_in, columns=col_names, index=index) | ||
|
|
||
| return df_out | ||
|
|
||
|
|
||
| def _numpy_to_series(obj_in: Union[np.generic, np.ndarray], index=None) -> pd.Series: | ||
| """ | ||
| Converts a numpy object to a pandas Series | ||
|
|
||
| Parameters | ||
| ---------- | ||
| obj_in : the object to convert | ||
| index : array-like (optional); will set index on Series | ||
|
|
||
| Returns | ||
| ------- | ||
| df_out : the object converted to a pandas Series | ||
| """ | ||
| s_out: pd.Series = pd.Series(obj_in, index=index) | ||
|
|
||
| return s_out |
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.
Hi guys, I spent a good few hours going over this function, and I still can't decide what is best.
The thing is, if we are going to replace
is_dataframewith this function, then the function should be able to return a copy of the df (likeis_dataframe) and potentially a copy of y (not sure is necessary), to avoid inadvertently modifying the user's data. So before merging, at least, we need to ensure we return a copy of X.When playing around with this function, I found out some errors in the tests raised by check_estimator. One of the tests would be if y=None, and I've noticed that we do not handle this situation. At the moment, we are assuming that the user would enter a numpy array or a series, but in theory, they could also enter a list, a tuple or None, and all of that would work just fine with this version of check_pd_x_y and still fail in the concatenation, unless we ensure we return a pd.series from this function.
But, pd.Series(None) would still return a pd.series. And if we do pd.Series(None, index=X.index) it will return a pd series of the length of the dataframe full of nan, when the user enters None.
If I convert a numpy array to a df, and force the index of the dataframe, what if the array was shorter than the df? would it fail or would it introduce Nan? I did not check this, so thinking out loud.
I then went ahead and checked the check_x_y function from sklearn for some inspiration, and I see that they handle the y=None at the very top of the check. So we probably should do that as well.
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.
Pretty sure I check for incompatible dimensions and unit test it, but I'll double check.
I'll go ahead and make the following changes:
Sound good?