-
Notifications
You must be signed in to change notification settings - Fork 91
Raise ValueError when user attempts to use .y for a component that does not produce a y output
#2662
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2662 +/- ##
=======================================
+ Coverage 99.9% 99.9% +0.1%
=======================================
Files 298 298
Lines 27305 27320 +15
=======================================
+ Hits 27261 27276 +15
Misses 44 44
Continue to review full report at Codecov.
|
| component_class = handle_component_class(component_info[0]) | ||
| self.component_instances[component_name] = component_class | ||
|
|
||
| self._validate_component_dict_edges() |
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.
Since this requires instantiation of components (to more easily handle detecting if the component is a subclass of a target transformer that would return a tuple), I had to create a separate helper method that was separate from self._validate_component_dict()
| graph = { | ||
| "Imputer": [Imputer, "X", "y"], | ||
| "OHE": [OneHotEncoder, "Imputer.x", "Imputer.y"], | ||
| "OHE": [OneHotEncoder, "Imputer.x", "y"], |
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 new logic catches some cases where we created a graph and used a component's .y when it doesn't produce such!
eccabay
left a comment
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.
LGTM!
freddyaboulton
left a comment
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.
Thank you @angela97lin !
bchen1116
left a comment
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.
LGTM! Agreed with Freddy's comments!
Closes #2569