Remove _needs_fitting attribute from Components
#398
Conversation
Codecov Report
@@ Coverage Diff @@
## master #398 +/- ##
==========================================
+ Coverage 97.45% 97.47% +0.01%
==========================================
Files 105 105
Lines 3267 3282 +15
==========================================
+ Hits 3184 3199 +15
Misses 83 83
Continue to review full report at Codecov.
|
jeremyliweishih
changed the title
_needs_fitting attribute from Components_needs_fitting attribute from Components
jeremyliweishih
changed the title
_needs_fitting attribute from Components_needs_fitting attribute from Components
jeremyliweishih
requested review from
angela97lin,
dsherry and
christopherbunn
and removed request for
angela97lin
evalml/pipelines/components/transformers/feature_selection/feature_selector.py
Show resolved
Hide resolved
evalml/pipelines/components/transformers/feature_selection/feature_selector.py
Outdated
Show resolved
Hide resolved
|
Good stuff. Left some comments, in particular having to do with |
| transformer.fit_transform(X) | ||
| with pytest.raises(MethodPropertyNotFoundError, match="Transformer requires a transform method or a component_obj that implements transform"): |
There was a problem hiding this comment.
Hmm, this test makes me wonder: would it be confusing for a user who tries to call transformer.fit_transform(X) to instead get an error that says ""Transformer requires a transform method or a component_obj that implements transform" instead? Perhaps stack trace is enough to make this clear? :O
| self.fit(X, y) | ||
| X_t = self.transform(X, y) | ||
| except MethodPropertyNotFoundError as e: | ||
| raise e |
There was a problem hiding this comment.
+1 to this. Let's make sure we have test coverage for:
- Component has valid
fit_transformmethod - Component has
fit_transformmethod but it throws an exception, like aRuntimeError - Component has no
fit_transformmethod, but fit and transform work - Component has no
fit_transformmethod, andfitortransformthrow
|
|
||
| Arguments: | ||
| X (pd.DataFrame): Data to transform | ||
| y (pd.Series, optional): Input Labels |
There was a problem hiding this comment.
I think the optional should go somewhere after the colon?
There was a problem hiding this comment.
@dsherry optional inside the parentheses is currently our convention in the codebase. It would be easier to keep this as is and keep track of the convention in a separate issue if we want to change it.
Fixes #366