-
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
[Merge After 0.6.1] Adding support for str/list input for classification #2541
[Merge After 0.6.1] Adding support for str/list input for classification #2541
Conversation
Job PR-2541-4487c0d 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 with a minor question. If a list contains invalid image_path, will it cause error in processor?
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, let's merge it after 0.6.1.
if contains_valid_images(data): | ||
data_dict = {"image": data} |
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.
Is it necessary to check if a list of image paths and use header image
? Whether image list or not doesn't affect creating the dataframe. pandas creates a dataframe with header 0
. I don't think header image
is required in later data processing and model forward.
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.
let me double check. I think I saw at some point the code actually uses the header image
@@ -515,3 +526,38 @@ def infer_dtypes_by_model_names(model_config: DictConfig): | |||
fallback_dtype = list(allowable_dtypes)[0] | |||
|
|||
return allowable_dtypes, fallback_dtype | |||
|
|||
|
|||
def contains_valid_images(data: Union[str, list], sample_n: Optional[int] = 50) -> bool: |
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.
How about reusing is_imagepath_column()
in infer_types.py
?
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 is_imagepath_column checks on a df.Series. This test was for array of images. That being said, let me check if the image
header is needed. If not, I can convert the image array to pd.Series and re-use is_imagepath_column()
.
elif isinstance(data, str): | ||
data = load_pd.load(data) | ||
if contains_valid_images(data): | ||
data_dict = {"image": [data]} |
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.
Similarly, header image
seems unnecessary.
Issue #, if available:
Description of changes: Adding support for str/list input for classification. Unit Test passed.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.