Skip to content
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

Set mem_cache_img_min_size to input_size if it's none #3842

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

eunwoosh
Copy link
Contributor

@eunwoosh eunwoosh commented Aug 14, 2024

Summary

This PR inlcudes

  • change update_mem_cache_img_max_size to update_mem_cache_img_min_size
  • set update_mem_cache_img_min_size to value of input_size if it's None.
  • set update_mem_cache_img_min_size to None if tiling

Details

change update_mem_cache_img_max_size to update_mem_cache_img_min_size

The reason why trying to resize image smaller before caching is to reduce image save size for mem cache.
And it should be bigger than final image size after transforms. If not, some image information can't be distorted or removed.
But current update_mem_cache_img_max_size implementation is focusing on maximum input size while keeping image ratio.
It means that if image shape is (100, 1000) and update_mem_cache_img_max_size is (200, 200) then image is resized to (20, 200) which can make unexpected result.
This PR changes it to guarantee that image isn't resized smaller than update_mem_cache_img_min_size

set update_mem_cache_img_min_size to None if tiling

tiling task needs to tile original images, so if image is resized before caching, it makes problem while tiling data processing.

How to test

Checklist

  • I have added unit tests to cover my changes.​
  • I have added integration tests to cover my changes.​
  • I have ran e2e tests and there is no issues.
  • I have added the description of my changes into CHANGELOG in my target branch (e.g., CHANGELOG in develop).​
  • I have updated the documentation in my target branch accordingly (e.g., documentation in develop).
  • I have linked related issues.

License

  • I submit my code changes under the same Apache License that covers the project.
    Feel free to contact the maintainers if that's a concern.
  • I have updated the license header for each file (see an example below).
# Copyright (C) 2024 Intel Corporation
# SPDX-License-Identifier: Apache-2.0

@github-actions github-actions bot added TEST Any changes in tests OTX 2.0 labels Aug 14, 2024
@eunwoosh eunwoosh changed the title Set update_mem_cache_img_max_size to input_size if it's none Set mem_cache_img_max_size to input_size if it's none Aug 14, 2024
@eunwoosh eunwoosh marked this pull request as ready for review August 14, 2024 08:43
goodsong81
goodsong81 previously approved these changes Aug 14, 2024
Copy link
Contributor

@goodsong81 goodsong81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the prompt update!

sungchul2
sungchul2 previously approved these changes Aug 16, 2024
@eunwoosh eunwoosh marked this pull request as draft August 16, 2024 05:59
@eunwoosh eunwoosh dismissed stale reviews from sungchul2 and goodsong81 via 37b39a3 August 16, 2024 06:00
@eunwoosh eunwoosh force-pushed the update_mem_cache_img_max_size branch from 37b39a3 to 45f345c Compare August 23, 2024 05:26
@eunwoosh eunwoosh changed the title Set mem_cache_img_max_size to input_size if it's none Set mem_cache_img_min_size to input_size if it's none Aug 23, 2024
Copy link
Contributor

@goodsong81 goodsong81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate why you changed max -> min?
max might be semantically right as the cache would downscale if input image is larger than the criteria which is the upper limit.

@eunwoosh
Copy link
Contributor Author

Could you elaborate why you changed max -> min? max might be semantically right as the cache would downscale if input image is larger than the criteria which is the upper limit.

I thought, but when I applied to set mem_cache_img_max_size to input_size, I changed my thought. I think image shouldn't become smaller than final image size because there is possibility that image information can disappear. But it's not easy to guarantee if max is only choice we have (Please refer what I wrote above). So, I think providing a way to guarantee that is more useful than max.

@goodsong81
Copy link
Contributor

Could you elaborate why you changed max -> min? max might be semantically right as the cache would downscale if input image is larger than the criteria which is the upper limit.

I thought, but when I applied to set mem_cache_img_max_size to input_size, I changed my thought. I think image shouldn't become smaller than final image size because there is possibility that image information can disappear. But it's not easy to guarantee if max is only choice we have (Please refer what I wrote above). So, I think providing a way to guarantee that is more useful than max.

Do you upscale small input image if it's smaller than the mem_cache_img_min_size?
The name implies so, but it might be misleading whether it's yes or no.

Cache should generally have 'upper' bound than 'lower' bound.
We could have set a right or enough large size according to the final input size.

  • image size: 100x100 / cache max: 1000x1000 / input size: 500x500 -> 100x100 cached
  • image size: 2000x2000 / cache max: 1000x1000 / input size: 500x500 -> 1000x1000 cached
  • image size: 2000x2000 / cache max: 500x500 / input size: 500x500 -> 500x500 cached

In sum, it would be better to change parameter value, not the semantic logic of memcache.
But feel free to go ahead with you design choice if others agree :)

@sovrasov sovrasov removed the OTX 2.0 label Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TEST Any changes in tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants