-
Notifications
You must be signed in to change notification settings - Fork 1
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
Warning when fitting transformed estimator with dataframe with no feature names #35
Comments
This overhauls the feature validation to dramatically simplify changes that were introduced by #22 and #34. I think the easiest way to explain the changes of this commit are to explain how we ended up with that complexity. In #22, we needed to ensure that our estimators got feature names when fitted with dataframe inputs. This was complicated by the fact that they received transformed inputs, and our transformers cast dataframes to arrays, dropping the names. We dealt with this using a `NamedFeatureArray` class that would retain names after being cast. In #34, we implemented the `set_output` API and used this instead to ensure that our transformers returned dataframe outputs. At the same time, we decided that estimator feature names should match the names that original names, not the transformed names, and implemented the `feature_names_in_` property. What we failed to realize is that the `feature_names_in_` property removed any need to extract feature names from the transformed inputs because that extraction is handled entirely by the transformers! This removed the need to use the `set_output` API. The other major change is that the `_check_feature_names` method that we overrode in #34 does not need to be implemented directly because we can instead use that method from the transformer (via `_validate_data`) without having to worry about mismatched feature names during fitting.
@aazuspan, great explanation of the issue and I understand what's going on. Correct me if I'm wrong, but this definitely feels like a bit of a corner case, in that a user would have to read in I'm a bit conflicted over what the expected behavior should be. One argument would be that given that the user (intentionally/unintentionally) passed |
I think you're right that this is definitely an edge case, and probably not worth worrying about beyond maintaining consistency with sklearn. However... While trying to solve this, I realized that I added a ton of unnecessary complexity in #34. This commit message goes into some depth, but essentially I think I got stuck on the idea that we needed the transformer to pass dataframes to our estimator so that we could get feature names, but that became totally unnecessary once we added the Removing that requirement simplifies the With those gone, and after a few hours tracing through exactly what methods get called with what data, I realized that we could also simplify
Trying to accomplish both of those objectives (don't warn when fitting but do warn when predicting) was why I had to copy some of the logic from the sklearn implementation. However, I realize now that we can avoid that complexity (and the licensing) by just disabling that check and instead running that check via the transformer in Sorry for the whiplash of adding a bunch of features then pulling them all out again! I realize this is a lot to digest, so I'm happy to clarify or discuss further. But if this all makes sense and you don't see any glaring holes in my logic, I'm ready to make a PR that would fix this and simplify the validation. |
I'm so glad you understand everything, but I'm right at about 30-40% on this 😄. I'm going to take the cop-out that I generally understand how you've simplified things (and the code is much simpler) and trust that you have a good handle on all of this. I'm good with you moving forward with the PR and promise to stare at it more such that I get to 60-80%! |
That's fair enough! |
This overhauls the feature validation to dramatically simplify changes that were introduced by #22 and #34. I think the easiest way to explain the changes of this commit are to explain how we ended up with that complexity. In #22, we needed to ensure that our estimators got feature names when fitted with dataframe inputs. This was complicated by the fact that they received transformed inputs, and our transformers cast dataframes to arrays, dropping the names. We dealt with this using a `NamedFeatureArray` class that would retain names after being cast. In #34, we implemented the `set_output` API and used this instead to ensure that our transformers returned dataframe outputs. At the same time, we decided that estimator feature names should match the names that original names, not the transformed names, and implemented the `feature_names_in_` property. What we failed to realize is that the `feature_names_in_` property removed any need to extract feature names from the transformed inputs because that extraction is handled entirely by the transformers! This removed the need to use the `set_output` API. The other major change is that the `_check_feature_names` method that we overrode in #34 does not need to be implemented directly because we can instead use that method from the transformer (via `_validate_data`) without having to worry about mismatched feature names during fitting.
Resolved by #36 |
This might set a record for the shortest time between merging a PR and finding a new bug that it caused...
When fitting a transformed estimator with a dataframe without feature names,
_check_feature_names
incorrectly warns:UserWarning: X has feature names, but GNNRegressor was fitted without feature names
.To reproduce:
This wasn't caught by our tests because we specify column names for all of our dataframe inputs, and this will only occur when the dataframe has no column names. What happens is that
est.transform_
gets fit with a dataframe with no feature names, but outputs a dataframe that does have feature names (in this case, [cca0
,cca1
]). Once fit works its way through the MRO toKNeighborsRegressor.fit
, it calls_validate_data
which calls our overridden_check_feature_names
that complains about the mismatched feature names.A quick fix for this is to change the condition under which we use
pandas
output mode on the transformer so that we only use it if sklearn can extract feature names, but I feel like that whole section might be unnecessarily complex, so I'll give this a little more thought before I push anything...The text was updated successfully, but these errors were encountered: