[Refactor][Bug fix] Fix fit_summary + move problem_type outside predictor.py #2578
Conversation
|
|
||
| name: str # Name of the problem | ||
| support_fit: bool = True # Whether the problem type support `.fit()` | ||
| inference_ready: bool = False # Support `.predict()` and `.evaluate()` without calling `.fit()` |
There was a problem hiding this comment.
I think inference_ready is better to be a state of predictor.
There was a problem hiding this comment.
This indicates whether the problem supports zero-shot inference.
There was a problem hiding this comment.
How about using name support_zero_shot? I think we may need inference_ready in predictor in later refactoring.
There was a problem hiding this comment.
Sounds good. support_zero_shot is reasonable.
|
Job PR-2578-27d2029 is done. |
|
|
||
| Usually you do not need to preprocess the text / image data. AutoGluon Multimodal has built-in | ||
| support of text / image preprocessing. However, this won't block you from appending custom preprocessing logic before | ||
| feeding in the dataframe to AutoGluon Multimodal. |
There was a problem hiding this comment.
However, this won't block you from appending custom preprocessing logic before feeding in the dataframe to AutoGluon Multimodal. Just want to confirm, if user already preprocess the image (e.g., minus 255, divided by customized mean, or data augmentation), will AutoGluon do another round of preprocessing and data augmentation?
There was a problem hiding this comment.
we will do another round of normalization. As in
There was a problem hiding this comment.
I can add a flag to disable normalization in a later PR. I think this is a good point.
| from typing import OrderedDict as t_OrderedDict | ||
| from typing import Union |
There was a problem hiding this comment.
The two lines are not used?
There was a problem hiding this comment.
t_OrderedDict is used for type hint. Will remove Union.
| @@ -1152,7 +1119,7 @@ def _fit( | |||
| ) | |||
|
|
|||
| config = update_config_by_rules( | |||
| problem_type=self._problem_type, | |||
| problem_type=self.problem_type, | |||
There was a problem hiding this comment.
I see all self._problem_type are replaced by self.problem_type. Mind to tell the reasons?
There was a problem hiding this comment.
Just to access it via the property call. Both self._problem_type and self.problem_type should be okay.
There was a problem hiding this comment.
Right. Currently, self.problem_type has the same value as self._problem_type. But since the property is more user-faced, there may be some cases they are not exactly the same. I think using self._problem_type internally may be a better choice to avoid possible changes.
There was a problem hiding this comment.
I can also change all self.problem_type to self._problem_type so that it will be consistent.
There was a problem hiding this comment.
One advantage of self.problem_type over self._problem_type is that you can never use self.problem_type = XXX which accidentally overwrite the value.
|
Job PR-2578-88c338f is done. |
| support_zero_shot=True, | ||
| is_matching=True, | ||
| supported_modality_type={TEXT}, | ||
| supported_label_type={CATEGORICAL, NUMERICAL}, |
There was a problem hiding this comment.
For matching problems, labels can be absent. Will it support this case?
There was a problem hiding this comment.
For those case, we can treat the label type as BINARY. But we need to submit a follow-up PR to refactor the code-base to better use these flags.
There was a problem hiding this comment.
The logic of no label is different from binary label. We'd better distinguish between them.
There was a problem hiding this comment.
Actually there are multiple concepts: modality type, column type and problem type. This is the first PR to split the problem type into a separate file. Need to submit follow-up PRs.
There was a problem hiding this comment.
Matching itself also needs major refactor so I haven’t touched it extensively
| IMAGE_SIMILARITY, | ||
| IMAGE_TEXT_SIMILARITY, | ||
| MULTICLASS, | ||
| NAMED_ENTITY_RECOGNITION, |
There was a problem hiding this comment.
I do not recall I used this constant: "NAMED_ENTITY_RECOGNITION". Shall we use NER only?
There was a problem hiding this comment.
Both NER and NAMED_ENTITY_RECOGNITION will be mapped to NER.
There was a problem hiding this comment.
You can view NAMED_ENTITY_RECOGNITION as the alias of NER problem type.
|
Job PR-2578-69f61a0 is done. |
Issue #, if available:
Description of changes:
predictor.fit_summary()won't work after you have loaded the predictor from folder.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.