Fix rescale normalize inconsistencies in fast image processors #36388
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
| @@ -587,17 +588,17 @@ def preprocess( | |||
| image_mean = tuple(image_mean) if isinstance(image_mean, list) else image_mean | |||
| image_std = tuple(image_std) if isinstance(image_std, list) else image_std | |||
|
|
|||
| image_mean, image_std, interpolation = self._prepare_process_arguments( | |||
| image_mean, image_std, do_rescale, interpolation = self._prepare_process_arguments( | |||
There was a problem hiding this comment.
I would better unpack this to make it explicit.. because it's still confusing what happens inside the function + we are mixing two responsibilities validation and modification, which is not the best practice overall and can cause inability to use this function for other non-standard processors.
BTW, any reason not for doing image_mean, image_std, rescale_factor fusion inside rescale_and_normalize function?
There was a problem hiding this comment.
Agreed, I'll split this in more functions.
BTW, any reason not for doing image_mean, image_std, rescale_factor fusion inside rescale_and_normalize function?
Yes the goal was to not recompute "rescaled" image_mean and image_std at every call to preprocess, so computing them outside of rescale_and_normalize allows to use lru_cache
There was a problem hiding this comment.
Ok, we can also create a separate function and use it within rescale_and_normalize. I apologize if I seem a bit pushy, but it seems logical to assign this responsibility to the transform itself. This approach is fairly standard in other libraries (e.g., Albumentations), so it's better to follow a common pattern. Anyway, I'm happy to discuss if you think it's better to keep it separate.
@lru_cache
def fuse_mean_std_and_rescale_factor(mean: Tuple[float, ...], std: Tuple[float, ...], factor: float):
...There was a problem hiding this comment.
No that sounds better indeed, Thanks!
| # Fused rescale and normalize | ||
| image = self.rescale_and_normalize(image, do_rescale, rescale_factor, do_normalize, image_mean, image_std) |
| @@ -260,7 +260,7 @@ def preprocess( | |||
| image_mean = tuple(image_mean) if isinstance(image_mean, list) else image_mean | |||
| image_std = tuple(image_std) if isinstance(image_std, list) else image_std | |||
|
|
|||
| image_mean, image_std, interpolation = self._prepare_process_arguments( | |||
| image_mean, image_std, do_rescale, interpolation = self._prepare_process_arguments( | |||
There was a problem hiding this comment.
I suppose this should be adjusted
|
P.S. ahh, ok, I got your comment |
| if size is not None: | ||
| kwargs["size"] = SizeDict(**get_size_dict(size=size, default_to_square=default_to_square)) | ||
| if crop_size is not None: | ||
| kwargs["crop_size"] = SizeDict(**get_size_dict(crop_size, param_name="crop_size")) | ||
| if isinstance(image_mean, list): | ||
| kwargs["image_mean"] = tuple(image_mean) | ||
| if isinstance(image_std, list): | ||
| kwargs["image_std"] = tuple(image_std) | ||
| if data_format is None: | ||
| kwargs["data_format"] = ChannelDimension.FIRST |
There was a problem hiding this comment.
if you modify them, why use kwargs and not an arg? Makes more sense to me
|
Also we should not have to get each kwarg individually to pass them to the validation, this defeats the kwargs usage |
…ze-inconsistencies
…ze-inconsistencies
What does this PR do?
Improve handling of fused rescale and normalize, allow normalizing of integers tensors.
As dicsussed with @qubvel