-
Notifications
You must be signed in to change notification settings - Fork 861
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
Remove pipeline from matcher initialization API #2569
Conversation
Is it a temporary solution? We should later unify the design of |
Job PR-2569-c54b2cc is done. |
According to our general design discussions, predictor may need the |
PR updated. Now matcher uses only |
Job PR-2569-d423d4b is done. |
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
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
@@ -501,3 +502,7 @@ def convert_data_for_ranking( | |||
response_data = pd.DataFrame({response_column: data[response_column].unique().tolist()}) | |||
|
|||
return data_with_label, query_data, response_data, label_column | |||
|
|||
|
|||
def is_matching(pipeline: str): |
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.
Shall we use
problem_property_dict = OrderedDict( |
problem_property_dict[pipeline].is_matching
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.
There is circular import issue if importing problem_property_dict
from predictor.py
. We may refactor this later.
self._problem_type = problem_type.lower() if problem_type is not None else None | ||
self._pipeline = pipeline.lower() if pipeline is not None else None | ||
self._problem_type = None # always infer problem type for matching. | ||
self._pipeline = problem_type.lower() if problem_type is not None else None |
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.
In the next PR, we can remove pipeline
.
We can refactor the implementation further after #2578 |
Job PR-2569-f723802 is done. |
Issue #, if available:
Need to align some details between matcher and predictor before fully merging them.
Description of changes:
pipeline
from the matcher initialization API.pipeline
frominfer_metrics()
. We still need to know whether it's a matching task. But no longer require the data modalities in matching.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.