-
Notifications
You must be signed in to change notification settings - Fork 87
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
Standardize error when calling transform/predict before fit for pipelines #1048
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1048 +/- ##
=======================================
Coverage 99.91% 99.91%
=======================================
Files 188 191 +3
Lines 10296 10362 +66
=======================================
+ Hits 10287 10353 +66
Misses 9 9
Continue to review full report at Codecov.
|
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.
Looks good! Just some suggestion on adding a test case and also how we could move the two metaclasses into one but not blocking.
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.
@angela97lin Looks good! I just left two minor comments.
evalml/pipelines/base_meta.py
Outdated
@wraps(method) | ||
def _check_for_fit(self, X=None, y=None): | ||
klass = type(self).__name__ | ||
if not self._is_fitted and klass and self.needs_fitting: |
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.
Why do we need the and klass
?
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.
Oops we don't, glad you caught this 😅
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.
@angela97lin looks good!
What do you think of this:
- Define
BaseMeta
inevalml/utils/base_meta.py
- Define
ComponentBaseMeta
inevalml/pipelines/components/component_base_meta.py
- Define
PipelineBaseMeta
inevalml/pipelines/pipeline_base_meta.py
Approved once we confirm organization.
@dsherry Sure, that sounds fine to me, could be useful if we use BaseMeta for anything else. I'll update this PR accordingly. |
Closes #988