Return None for unreadable images and try to infer num channels #1307
Conversation
| @@ -30,16 +31,15 @@ | |||
| logger = logging.getLogger(__name__) | |||
|
|
|||
|
|
|||
| @functools.lru_cache(maxsize=32) | |||
There was a problem hiding this comment.
Is the purpose this cache to avoid downloading the image twice when we infer the image dimensions (1) and then process it (2)?
There was a problem hiding this comment.
Yeah, or to avoid downloading images with the same url in each step
|
|
||
| height, width = round(height_avg), round(width_avg) | ||
| logger.debug("Inferring height: {0} and width: {1}".format(height, width)) | ||
| elif first_image is not None: |
There was a problem hiding this comment.
It looks like we can generalize this first_image code path as a special case of the infer_image_dimensions case. Specifically, what if we removed the infer_image_dimensions param and just used infer_image_sample_size. Then if the user sets this value to 1, we get the same effect as setting infew_image_dimensions=False, right?
There was a problem hiding this comment.
actually I just realized there is a slight difference -infer_image_dimensions also sets should_resize = True, which will prevent Ludwig from throwing an error on the first mismatch - only having infer_image_sample_size=1 will mean that a size mismatch error will never be thrown because everything will be silently resized
There was a problem hiding this comment.
I see. In this case, I would be in favor of making should_resize=True the default (because the default is now to infer image dimensions). Then if the user wants the old behavior, they would need to set should_resize=False manually. An additional thing we can do in this case, when should_resize=False and infer_image_sample_size > 1 is raise the error if not all the elements of the sample match. Would that solve the issue?
There was a problem hiding this comment.
By user setting the older behavior, do you mean exposing should_resize and user_specified_num_channels through preprocessing parameters? Or meaning they would just edit should_resize = False in the code.
If we expose should_resize and user_specified_num_channels through preprocessing parameters, then those would just be replacing infer_image_dimensions, so I don't know if that would decrease complexity. There also could be conflicts (i.e. user specifies explicit width and height but then has should_resize set to False.
One possible solution is to make infer_image_sample_size = 0 the old case, to not infer anything and fail on any mismatch, and then anything > 0 would implicitly mean should_resize = True, so we wouldn't need any other parameters. The only question is if = 0 is intuitive enough to encode this information
There was a problem hiding this comment.
Hmm, yeah I see the issue here. I think given how much we would need to overhaul the config params to make it work, it's probably fine to leave it as-is for now.
No description provided.