Check X and y index match when encoding, closes #376 by noahjgreen295 · Pull Request #377 · feature-engine/feature_engine

noahjgreen295 · 2022-02-19T16:54:26Z

solegalli

Thank you so much for this changes. The code looks good.

The OrdinalEncoder also takes y if using the "ordered" strategy. Could you check if we need this function there as well?

There are other transformers in feature-engine that also require y, like the decision tree discretisers and decision tree encoder. But I guess, since they do not concatenate, they would not need this fix? Did you check?

I have a minor suggestion to re-structure the tests as well. Could you have a look?

Thank you!

solegalli · 2022-02-20T15:57:07Z

@@ -0,0 +1,54 @@
+import numpy as np
+import pandas as pd


Can we move the code in this file to this other file were we have the common tests done to more than 1 encoder?

definitely - I had not noticed that file earlier and it is certainly where it belongs. Fixed in latest commit.

solegalli · 2022-02-20T16:12:17Z

+
+    # Will serve as a no-op whose chief purpose is to turn the
+    # X into an np.ndarray
+    si = SimpleImputer(strategy="constant", fill_value="a")


Can we not just convert the dataframe into an array without calling the simple imputer? it should be faster. Because here we are using the simple imputer just to reset the index and obtain an array from a dataframe. So we could in theory do this with numpy. And then we have less dependencies.

In fact, we could do the following instead:

create a dataframe X with some index and a y is an array

create an array X and y is a series.

And we test both, avoiding the simple imputer and the assert types below. Am I correct?

Definitely agreed about getting rid of SimpleImputer and shortening code; done in latest commit.

However, in terms of this case you mentioned:

create a dataframe X with some index and a y is an array
I wasn't sure how that situation might come about. I see @bmreiniger discussing similar below so will ask there.

I am thinking, if a user created a target like this y = np.where(df['my_var']=='yes', 1, 0), that would return an array.

The truth is, I am not sure how often this happens. But at the moment, most of our transformers do not enforce y to be a series. So users could, in practice, pass a numpy array. So it might be worth to expand the check, so that transformers work for them as well.

But then again, I am not so certain about how often this could happen. What is your experience @bmreiniger @noahjgreen295 ?

Alright, I did some research: most of our transformers that take y, would use a Scikit-learn estimator at the back, so for those, the Check_X_y from sklearn would take care of X and y and we have nothing to worry about.

The only transformers that might have a problem are the encoders, woe, meantargetencoder, probabilityratio, like @noahjgreen295 sayd and I think the ordinal(encoding_method="ordered") should have it too (more below).

Those transformers, if they receive an array for y, they would transform it to a series, and then it would be reindexed from 0. Likely, the index of the converted series will not match the train df index. But if this is the case, the logic in the check_x_y_mismatch as it is now, should handle it. But, note that we would be giving the df an index that is reindexed from 0.

So I would say, let's add a test for when X is dataframe, and y is an array, just to corroborate that what I am saying is true. But I don't think we need to change the logic of the check_x_y_mismatch.

Also, the ordinalencoder(encoding_method="ordered") uses exactly the same logic as all other encoders. So I am not sure why that transformer would not fail the test if we do not add the check_x_y mistmatch. If you added a test to see that it passes, that would be great.

bmreiniger · 2022-02-20T16:33:03Z

It won't come up as often, but I wonder these two other cases should be considered:

pandas X (with nonstandard index) and numpy y
pandas X and y, but with mismatched indexes

The former is reasonably straightforward, but the second requires a decision: should it error, or warn and reindex, or silently reindex, or operate as though the rows in X and y really don't line up (probably also with a warning)?

solegalli · 2022-02-20T16:52:26Z

y is supposed to be the observation-wise data for X, so they should also match in lenght.

We have a separate issue to add a check for x and y lenght mismatch #365 in case you guys have some time in your hands :)

But to @bmreiniger 's suggestion, I think we should act without a warning, lol. Probably silently re-index.

noahjgreen295 · 2022-02-21T22:57:45Z

Hi - will definitely take care of these changes.
I am on holiday for remainder of this week but will take a look this coming weekend - sorry for the delay!

solegalli · 2022-02-21T23:55:09Z

Hi @noahjgreen295

No problem at all. Enjoy your holidays!

noahjgreen295 · 2022-02-22T03:21:37Z

Hi @noahjgreen295

No problem at all. Enjoy your holidays!

Thanks! Will try to do a little bit now as well.

noahjgreen295 · 2022-02-22T03:50:51Z

ww

It won't come up as often, but I wonder these two other cases should be considered:

pandas X (with nonstandard index) and numpy y

pandas X and y, but with mismatched indexes

The former is reasonably straightforward, but the second requires a decision: should it error, or warn and reindex, or silently reindex, or operate as though the rows in X and y really don't line up (probably also with a warning)?

Re this case

pandas X (with nonstandard index) and numpy y
When would this come up in the course of preceding sklearn transforms. I could certainly see this happening if the data began this way before the transforms, in which case I think it's a pre-existing issue in the user's code and we would not really be obligated to handle. But if there is some way to get into this state because of preceding transforms, let me know the case & I can write code to handle.

Re this case

pandas X and y, but with mismatched indexes
This certainly seems like a pre-existing issue with the user's data and not a side effect caused by sklearn transforms, but again let me know if not and I will handle. thx!

noahjgreen295 · 2022-02-22T04:01:36Z

The OrdinalEncoder also takes y if using the "ordered" strategy. Could you check if we need this function there as well?

Yes, tested this one - did not have the issue and did not require the fix.

There are other transformers in feature-engine that also require y, like the decision tree discretisers and decision tree encoder. But I guess, since they do not concatenate, they would not need this fix? Did you check?

I did test DecisionTreeEncoder and it did not have the issue. Once again am adding it to the test suite for coverage.
I just tested DecisionTreeDiscretiser and no issues there either. However I'm refraining from adding it to the test suite since this PR is focused on the encoding module.

solegalli · 2022-02-22T18:49:01Z

Thinking it further, the second example, where X and y have a different index yet they are both pandas objects: if we handled it silently, we might be doing more bad than good, because mis-aligned indeces might be a symptom of something going wrong somewhere up in the pipeline. I mean, why would they not match if they are 2 dataframes?

I could imagine that I re-indexed my train set, but forgot to reindex my y. That has indeed happened to me. But then, that should be on the user to pick up. Otherwise, it is too risky. So maybe we let that one go for the moment?

What's your thoughts?

@bmreiniger @noahjgreen295

solegalli · 2022-03-14T15:48:59Z

Hi guys @noahjgreen295 @bmreiniger

Just checking in to see if, by any chance, you would have time to finish this PR?

Not long to go I believe :)

thank you!

noahjgreen295 · 2022-03-22T15:39:37Z

Just checking in to see if, by any chance, you would have time to finish this PR?

Sorry I've been away! Dealing with some job-related disruption that came after returning from vacation. Things are settling down now - I should be able to take care of by early next week. Sorry for this delay!

noahjgreen295 · 2022-03-27T15:56:34Z

Hi - I'm back on the case :-) Sorry again for delay.
Before resolving the issues you raised, I actually came across another related and possibly bigger issue.
Until now, my unit test was a somewhat contrived example in that X had only 1 column. But what if it has more than 1, which is the main use case? When X gets turned into an ndarray upstream, not only does it lose its index, it also loses its column names. So even when it gets turned back into a DataFrame, it doesn't have the column names, and the encoders cannot target specific columns using the variables parameter. I changed the unit test to illustrate this by failing, deliberately adding an the additional column incompatible with the encoders, while setting their variables parameter to avoid it. This setting doesn't work because of the disruption caused by converting to ndarray and losing the column names. All of this is making me wonder whether there really is a way to recover from upstream ndarrays. Let me know what you think - I'll be able to respond very quickly going forward :-)

bmreiniger · 2022-03-27T18:00:13Z

I think that's a big enough difference to warrant a separate issue and/or pull request. The indexes we can match up with y, but we have no good way to recover column names in the middle of a pipeline yet.

The sklearn solution to this is to use ColumnTransformers, though even there feature names won't appear if it's later in a pipeline. A user here can make this work by keeping track of feature indices and default names.

noahjgreen295 · 2022-03-27T21:07:38Z

I think that's a big enough difference to warrant a separate issue and/or pull request. The indexes we can match up with y, but we have no good way to recover column names in the middle of a pipeline yet.

The sklearn solution to this is to use ColumnTransformers, though even there feature names won't appear if it's later in a pipeline. A user here can make this work by keeping track of feature indices or default names.

Got it - will limit to current issue. thx

noahjgreen295 · 2022-03-27T21:22:31Z

Thinking it further, the second example, where X and y have a different index yet they are both pandas objects: if we handled it silently, we might be doing more bad than good, because mis-aligned indeces might be a symptom of something going wrong somewhere up in the pipeline. I mean, why would they not match if they are 2 dataframes?

I could imagine that I re-indexed my train set, but forgot to reindex my y. That has indeed happened to me. But then, that should be on the user to pick up. Otherwise, it is too risky. So maybe we let that one go for the moment?

What's your thoughts?

@bmreiniger @noahjgreen295

Sorry, forgot to respond here -
Re first use case (X is DataFrame, y is ndarray), you're right - I've been realizing there definitely are situations with y as ndarray and X as DataFrame. It kind of sort of seems it might usually be the user's fault but I think it's worth handling, so I will. Am about to commit a unit test showing it causes error, so will write the code to handle and pass the test.
Re second use case (both pandas, user mismatched indexes), agreed with you here as well - do not try to fix it as the user should be made aware of the error by the code failing.

noahjgreen295 · 2022-03-28T01:46:50Z

OK all relevant changes posted. Remaining tasks:

Find out why OrdinalEncoder doesn't have this problem. It would be easy to just add the same checks to that code just to be safe (I haven't done that yet since it is passing tests), but I still want to investigate to see why.
Standard work to get all CI tests to pass.
I'm seeing a conflict on this branch - not sure why as it doesn't appear in my local copy. Will resolve.

Should be able to get these done tomorrow or Tuesday.

bmreiniger · 2022-03-28T03:32:59Z

OrdinalEncoder indeed does have a problem. It's just that the incorrect joining causes the temp frame built in fit to be incorrect, so the resulting mapping is wrong, but still usable (no NaNs).

noahjgreen295 · 2022-03-28T10:43:13Z

OrdinalEncoder indeed does have a problem. It's just that the incorrect joining causes the temp frame built in fit to be incorrect, so the resulting mapping is wrong, but still usable (no NaNs).

Agreed - spotted same last night. Will modify the unit tests to look for expected return values rather than just non-NaN. Should definitely be able to get to that today.

noahjgreen295 · 2022-03-28T12:23:21Z

OK latest is pushed - I think we are there!

solegalli

Hi @noahjgreen295

Thank you for the code changes and the thorough testing.

I tried to flesh out the possible situations that we may encounter when working with feature-engine encoders, to better understand when we should raise an error and when we should not.

I wonder if what I wrote in #376 makes sense, and if yes, if we could incorporate all the scenarios in the X_y-check?

thank you!

solegalli · 2022-03-28T17:56:07Z

+    elif isinstance(X, (pd.DataFrame, pd.Series)) and isinstance(y, np.ndarray):
+        y = pd.Series(y)
+        y.index = X.index
+


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, is_dataframe converted it to a df, and if it was something else that is not permitted, like a pd.Series, is_dataframe should have raised an error (if it does not, we need to fix is_dataframe).

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 :_(

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?

I was thinking something like that earlier but was afraid to make changes to calls to _is_dataframe() :-) Now I know that's OK so will have a look tonight, from what I can tell this makes sense.

X can only be a pandas dataframe at this stage, because if it was an array, is_dataframe converted it to a df

In commits from earlier, I deliberately put the call to _check_X_y_pd_np_mismatch() before the call to self._check_fit_input_and_variables() (and thus _is_dataframe()) so that X has the chance to arrive at _check_X_y_pd_np_mismatch() as an array. (For an example, see here ) This allows me to detect that particular error case.

and if it was something else that is not permitted, like a pd.Series, is_dataframe should have raised an error (if it does not, we need to fix is_dataframe)

This is correct. I'll remove the logic that handles cases of X being a Series since that is not possible.

I do feel good about merging the functionality of _check_X_y_pd_np_mismatch() into self._check_fit_input_and_variables(). However, I think it might mean some restructuring of the unit tests, so please confirm.

I have additional followup on the #376 page.

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):

if self.encoding_method == "ordered": X, y = check_X_y_pd_np_mismatch(X, y) else: X = is_dataframe(X)

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.

noahjgreen295 · 2022-03-29T00:59:54Z

All changes done except whether to merge logic of _check_X_y_pd_np_mismatch() into another function (self._check_fit_input_and_variables()? , _is_dataframe()? etc.) or whether good as is.

solegalli

Thank you for the quick turnaround @noahjgreen295 !!! Much appreciated :)

I think we are almost there. As it stands now, due to code legacy, we are running the same functionality (is_dataframe(X)) twice in some transformers.

But if we change the code slightly here and there, we can make this more elegant and performant.

I would suggest the following:

check_x_y should return a df and a series and handle all possible ok cases, problematic cases and error cases (most of it is already there)
in base encoder: remove is_dataframe from _check_fit_input_and_variables which would now return self
for the encoders that do not require y, add is_dataframe when needed
for encoders that do require y, check_x_y handles everything, including transforming y into a series if necessary
OrdinalEncoder needs special attention because it should be able to work with and without y

Would you be up for these changes @noahjgreen295 ?

Thank you!

solegalli · 2022-03-29T09:21:03Z

+    1. X is an ndarray and y is a Series - converts X to DataFrame with y's index
+    2. X is a DataFrame and y is an ndarray - converts y to Series with X's index
+    3. X is a DataFrame and y is a Series, but their indexes don't match
+    - raises an error


This check is looking really good.

I would refactor it slightly so that it returns X and y as a Dataframe and Series, so we do not have additional work to do within the classes. So:

def _check_X_y_pd_np_mismatch( X: Union[pd.DataFrame, np.ndarray], y: Union[pd.Series, np.ndarray], ) -> Tuple[pd.DataFrame, pd.Series]:

The only missing functionality would be for when both X and y are arrays, that instead of being returned unchanged, we should return a df and a series.

solegalli · 2022-03-29T09:25:56Z

+
+    Returns
+    -------
+    X: changed as per description above


pd.Dataframe

solegalli · 2022-03-29T09:26:04Z

+    Returns
+    -------
+    X: changed as per description above
+    y: changed as per description above


solegalli · 2022-03-29T09:28:34Z

+    elif isinstance(X, (pd.DataFrame, pd.Series)) and isinstance(y, np.ndarray):
+        y = pd.Series(y)
+        y.index = X.index
+


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):

if self.encoding_method == "ordered": X, y = check_X_y_pd_np_mismatch(X, y) else: X = is_dataframe(X)

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.

solegalli · 2022-03-29T09:29:33Z

        )
+
+
+def _check_X_y_pd_np_mismatch(


I would call this method just _check_X_y()

solegalli · 2022-03-29T09:30:27Z

            check_classification_targets(y)

        # check input dataframe
+        X, y = _check_X_y_pd_np_mismatch(X, y)


Do we need this for this encoder? because X and y are being passed to cross_validate, and that should call for sklearn machinery to take care of X and y.

The test_detect_index_mismatch_from_x_pandas_y_pandas() unit test fails without it. I think it has to do with DecisionTreeEncoder using OrdinalEncoder but not sure. At any rate this call seems to be needed here to pass unit tests.

solegalli · 2022-03-29T09:31:34Z

        """

+        X, y = _check_X_y_pd_np_mismatch(X, y)
        X = self._check_fit_input_and_variables(X)


I would remove _is_dataframe from _check_fit_input_and_variables(X) which would now return self

this way we don't carry on the same process twice (check that X is a dataframe)

solegalli · 2022-03-29T09:32:14Z

+        X, y = _check_X_y_pd_np_mismatch(X, y)
        X = self._check_fit_input_and_variables(X)

        if not isinstance(y, pd.Series):


I would remove these lines of code, and I would make this part of check_X_y, to reduce boilerplate

solegalli · 2022-03-29T09:33:02Z

            Otherwise, y needs to be passed when fitting the transformer.
        """

+        X, y = _check_X_y_pd_np_mismatch(X, y)


I would restruture the logic here a bit:
if encoding_method is ordered, use check_x_y, otherwise use _is_dataframe()

solegalli · 2022-03-29T09:33:43Z

 def test_all_transformers(Estimator):
    return check_estimator(Estimator)
+
+


I think rebasing main would help solve the incompatibility with this file

noahjgreen295 · 2022-03-29T10:38:20Z

All of above makes sense to me - I can definitely do it! Should have either today. Thanks for looking through the codebase and pointing out the locations, that is a big help - much appreciated.

noahjgreen295 · 2022-03-30T13:00:52Z

All of above makes sense to me - I can definitely do it! Should have either today. Thanks for looking through the codebase and pointing out the locations, that is a big help - much appreciated.

Update - some deliverables for work coming up, so it will take a big longer but should have by end of week.

noahjgreen295 · 2022-04-02T21:12:23Z

Update - nearly done. Should have today or tomorrow. I did everything but older unit tests are breaking, I think because some of the new code is catching certain errors earlier and thus breaking the expectations of unit tests of downstream code. Should have sorted out soon.

noahjgreen295 · 2022-04-02T23:34:39Z

All set! Let me know what you think.

Re DecisionTreeEncoder - see response above
Re the merge conflict - so strange, I rebased to main and did push -f awhile ago, still getting that conflict.

Also:

circleci seems to be failing with the following message: attributeError: module 'contextlib' has no attribute 'nullcontext'. I'm using contextlib.nullcontext() in one of my unit tests. I'm using Python version 3.9.10; contextlib.nullcontext() was added in Python 3.7. Is the project and/or circleci using an earlier version of Python, or do you think this may be another issue?

…ix is done. Made it a separate .py file because it affects multiple encoders; parameterized it for each encoder with the known issue

…ough this function addresses issues that so far only pertain to some BaseEncoder subclasses, am putting it here as it may be useful for other situations

noahjgreen295 · 2022-04-03T01:07:08Z

Updating notes from above:

All set! Let me know what you think.

Re DecisionTreeEncoder - see response above
Re the merge conflict - resolved, I had not done a git pull upstream main in a long time. Just rebased, you can see it pushed all the commits into one series, if this a problem let me know.

… rebase

solegalli

Hi @noahjgreen295

Thank you so much. I like how it reads with the numpy functions you created.

The tests are failing in my local branch, there is some int vs float difference between the input and expected df both for dataframe checks and numpy.

And at the back of it, would it be possible to break the tests down into smaller tests that check a specific bit of the functionality? This way it is easier to debug whenever a test starts failing.

I added a comment below.

Thank you!

solegalli · 2022-04-04T15:57:32Z

+        ),
+    ],
+)
+def test_check_pd_X_y(


would it be possible to break this test into smaller tests that test one particular bit of the functionality? maybe the errors in one test? when both are numpy in a different test and so on?

The reason is, that tests over multiple functionality are difficult to debug. In fact, I changed slightly the logic of checK_pd_X_y and now I am finding hard to find out where the error is coming from.

Sure - that is always the balance between the fixtures and repeating code. At very least let me break out error conditions tests into one test, as you suggested. Will do right now.
Not sure why tests are failing on your branch, I'll do another git pull upstream to make sure I'm based off the same code. Will have all in a few minutes.

…ck_pd_X_y_both_same_type(), test_check_pd_X_y_np_to_pd(), and test_check_pd_X_y_errors()

noahjgreen295 · 2022-04-04T16:22:21Z

OK, pushed changes with test broken up into 3 new functions for easier distinction of errors.
All unit tests still running on my end, and I am based to latest code. Wonder if it's a version thing?

My versions are:
Python 3.9.10
Numpy 1.22.1
Pandas 1.3.4
feature-engine 1.2.0

bmreiniger · 2022-04-05T03:38:06Z

        self.n_features_in_ = X.shape[1]

-        return X
+        return self


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 self.

OK fixed in latest commit. Convention in rest of code for return self seems to be blank typehint and no mention in docstring, so I went with that.

@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:
https://stackoverflow.com/questions/43380042/purpose-of-return-self-python

Thank you!

@solegalli OK should I change to not return self?

solegalli · 2022-04-06T11:21:50Z

        )
+
+
+def _check_pd_X_y(


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_dataframe with this function, then the function should be able to return a copy of the df (like is_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.

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.

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:

Return copies of X and y when they are incoming as pandas objects

Raise an exception if y (or X) is None

Accommodate other array-like inputs for y

Sound good?

…sts to confirm

… empty; unit tests to confirm

noahjgreen295 · 2022-04-06T14:52:34Z

OK done. Let me know if these changes work.

noahjgreen295 · 2022-04-07T02:12:51Z

Hi - am working on those unit tests failing, should have tomorrow. I think it's all about the new code catching errors sooner than the sklearn check_estimator() can, so its error message assertions are failing. Will untangle all this soon.

solegalli · 2022-04-07T09:18:29Z

Hi - am working on those unit tests failing, should have tomorrow. I think it's all about the new code catching errors sooner than the sklearn check_estimator() can, so its error message assertions are failing. Will untangle all this soon.

FYI #410

I would appreciate your thoughts on that PR and if you have code change suggestions, could you PR to that branch please?

noahjgreen295 · 2022-04-09T22:11:01Z

Sorry, was delayed by some work stuff. Heading over to the new PR now!
So this one will be closed, correct?

solegalli · 2022-04-11T17:01:55Z

Sorry, was delayed by some work stuff. Heading over to the new PR now! So this one will be closed, correct?

It will close automatically when we merge the other one.

noahjgreen295 mentioned this pull request Feb 19, 2022

Encoders that are f(X, y) can produce nan results when y has non-standard index and X becomes an np.ndarray #376

Closed

solegalli reviewed Feb 20, 2022

View reviewed changes

solegalli changed the title ~~Fix issue 376~~ Check X and y index match when encoding, closes #376 Feb 22, 2022

solegalli reviewed Mar 28, 2022

View reviewed changes

solegalli reviewed Mar 29, 2022

View reviewed changes

noahjgreen295 added 2 commits April 2, 2022 20:39

created unit test reproducing issue 376; all test failing now until f…

4118156

…ix is done. Made it a separate .py file because it affects multiple encoders; parameterized it for each encoder with the known issue

added _check_for_X_y_index_mismatch() to dataframe_checks.py. Even th…

0d5e625

…ough this function addresses issues that so far only pertain to some BaseEncoder subclasses, am putting it here as it may be useful for other situations

noahjgreen295 added 7 commits April 2, 2022 20:42

black/isort/flake8

784dd21

additional flake8

d5ea00c

minor flake8

c3e8fd8

cleaned up some cruftiness in numpy_to_pandas

ad23215

black/isort/flake8

2337ca1

fixed incorrect merge that I had done after git pull upstream

5156bd1

black/isort/flake8

869d114

fixes to _check_pd_X_y() to fix unit test after git pull upstream and…

1d05f95

… rebase

solegalli reviewed Apr 4, 2022

View reviewed changes

noahjgreen295 added 2 commits April 4, 2022 12:18

broke up test_check_pd_X_y() into 3 separate test functions: test_che…

4f3c343

…ck_pd_X_y_both_same_type(), test_check_pd_X_y_np_to_pd(), and test_check_pd_X_y_errors()

black/isort/flake8

07e7a18

noahjgreen295 added 2 commits April 4, 2022 13:00

broke up further... both_same_type now both_numpy and both_pandas

e96af86

black/isort/flake8

fafbea0

bmreiniger reviewed Apr 5, 2022

View reviewed changes

fixed type hint and docstring on _check_fit_input_and_variables()

12cc894

solegalli reviewed Apr 6, 2022

View reviewed changes

noahjgreen295 added 5 commits April 6, 2022 10:35

confirmed that incompatible lengths also raise error in _check_pd_X_y

f953e91

_check_pd_X_y() now copies any incoming pandas objects; fixed unit te…

c24514b

…sts to confirm

_check_pd_X_y() raises exception if either incoming object is None or…

a92d4b0

… empty; unit tests to confirm

_check_pd_X_y() now supports list or Tuple for y; unit tests to confirm

709ac24

black/isort/flake8

f2b5ad9

solegalli mentioned this pull request Apr 7, 2022

add check_y and check_x_y to check that indexes of X and y match when encoding #410

Merged

solegalli closed this in #410 Apr 14, 2022

		def test_all_transformers(Estimator):
		return check_estimator(Estimator)

Uh oh!

Conversation

noahjgreen295 commented Feb 19, 2022

Uh oh!

solegalli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noahjgreen295 Feb 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmreiniger commented Feb 20, 2022

Uh oh!

solegalli commented Feb 20, 2022

Uh oh!

noahjgreen295 commented Feb 21, 2022

Uh oh!

solegalli commented Feb 21, 2022

Uh oh!

noahjgreen295 commented Feb 22, 2022

Uh oh!

noahjgreen295 commented Feb 22, 2022

Uh oh!

noahjgreen295 commented Feb 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

solegalli commented Feb 22, 2022

Uh oh!

solegalli commented Mar 14, 2022

Uh oh!

noahjgreen295 commented Mar 22, 2022

Uh oh!

noahjgreen295 commented Mar 27, 2022

Uh oh!

bmreiniger commented Mar 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

noahjgreen295 commented Mar 27, 2022

Uh oh!

noahjgreen295 commented Mar 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

noahjgreen295 commented Mar 28, 2022

Uh oh!

bmreiniger commented Mar 28, 2022

Uh oh!

noahjgreen295 commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

noahjgreen295 commented Mar 28, 2022

Uh oh!

solegalli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noahjgreen295 Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noahjgreen295 Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noahjgreen295 commented Mar 29, 2022

noahjgreen295 Feb 22, 2022 •

edited

Loading

noahjgreen295 commented Feb 22, 2022 •

edited

Loading

bmreiniger commented Mar 27, 2022 •

edited

Loading

noahjgreen295 commented Mar 27, 2022 •

edited

Loading

noahjgreen295 commented Mar 28, 2022 •

edited

Loading

noahjgreen295 Mar 28, 2022 •

edited

Loading

noahjgreen295 Mar 28, 2022 •

edited

Loading

noahjgreen295 commented Apr 2, 2022 •

edited

Loading

noahjgreen295 commented Apr 2, 2022 •

edited

Loading

noahjgreen295 commented Apr 3, 2022 •

edited

Loading