Add textimage #1848
Add textimage #1848
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new TextImage augmentation for rendering text on images, refactors augmentation class parameters into a new file for better organization, and updates tests to ensure compatibility and functionality. Additionally, the pre-commit configuration is updated to use ruff version v0.5.5. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://github.com/avatars/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ternaus - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 7 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
There was a problem hiding this comment.
Hey @ternaus - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Hardcoded file path for font detected. (link)
- Hardcoded file path for font detected. (link)
- Hardcoded file path for font detected. (link)
- Hardcoded file path for font detected. (link)
- Hardcoded file path for font detected. (link)
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🔴 Security: 5 blocking issues
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| if len(words_in_sentence) == 1: | ||
| assert result == sentence, "Single word input should remain unchanged" | ||
| else: | ||
| assert words_in_result != words_in_sentence or num_words == 0, f"Result should be different from input for n={num_words}" | ||
| assert len(words_in_result) == len(words_in_sentence), "Result should have the same number of words as input" | ||
| assert sorted(words_in_result) == sorted(words_in_sentence), "Result should contain the same words as input" |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if stopwords is None: | ||
| stopwords = ["and", "the", "is", "in", "at", "of"] |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if get_num_channels(image) in {1, 3}: | ||
| pil_image = ftext.convert_image_to_pil(image) | ||
| result = ftext.draw_text_on_pil_image(pil_image, metadata_list) | ||
| assert isinstance(result, Image.Image) | ||
| else: | ||
| result = ftext.draw_text_on_multi_channel_image(image, metadata_list) | ||
| assert isinstance(result, np.ndarray) |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| @@ -21,7 +20,7 @@ | |||
| from albumentations.core.types import ImageCompressionType | |||
| from albumentations.random_utils import get_random_seed | |||
| from albumentations.augmentations.transforms import RandomSnow | |||
| from tests.conftest import IMAGES, RECTANGULAR_FLOAT_IMAGE, SQUARE_FLOAT_IMAGE, SQUARE_MULTI_UINT8_IMAGE, SQUARE_UINT8_IMAGE | |||
| from tests.conftest import IMAGES, SQUARE_FLOAT_IMAGE, SQUARE_MULTI_UINT8_IMAGE, SQUARE_UINT8_IMAGE | |||
There was a problem hiding this comment.
issue (code-quality): Don't import test modules. (dont-import-test-modules)
Explanation
Don't import test modules.Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
| overlay_data: list[dict[str, Any]], | ||
| **params: Any, | ||
| ) -> np.ndarray: | ||
| return ftext.render_text(img, overlay_data, clear_bg=self.clear_bg) |
There was a problem hiding this comment.
Here can we do something like, please?:
return ftext.render_text(img, overlay_data, clear_bg=self.clear_bg), overlay_data
Because we need to keep track of the modified text. For example, in document parsing task, it is mandatory to have exactly matching annotations with their corresponding document images.
Summary by Sourcery
This pull request introduces a new 'TextImage' augmentation for rendering text on images, refactors test cases to include this new feature, and updates the build and documentation to support the new functionality.