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
Support for negative signs and optimization of exclusions #76
Conversation
@@ -76,14 +76,22 @@ def rotate_image(input_file, output_file, angle=90): | |||
:return: void | |||
Rotates image and saves result | |||
""" | |||
|
|||
if (angle == 0): |
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.
We should move those conditions out for separation of concerns:
the function should just rotate the image unconditionally; the logic whether to rotate the image should be outside.
Furthermore I have mixed feelings about reverting the sign of the rotation. What was the use-case for that?
So basically
if angle != 0:
rotate_image(input_file, output_file, angle)
What do you think? 😃
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.
As @monolidth said, this should rotate the image in the correct possition. Maybe it's not trivial.
receipt_parser/importer.py
Outdated
print(ORANGE + '\t~: ' + RESET + 'Rotate image' + RESET) | ||
with WandImage(filename=input_file) as img: | ||
with img.clone() as rotated: | ||
rotated.rotate(angle) | ||
rotated.save(filename=output_file) | ||
|
||
|
||
def sharpen_image(input_file, output_file): | ||
def sharpen_image(input_file, output_file, angle): |
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.
I can see that you added angle
to pass it to rotate_image
, but I think we should rather move rotate_image
out of sharpen_image
and remove the angle
parameter here. It's easier to test this way and again ensures separation of concerns.
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.
Updated this with last commit, thanks for your input! 👍
receipt_parser/importer.py
Outdated
@@ -212,9 +225,9 @@ def main(): | |||
img = grayscale_image(img) | |||
img = remove_noise(img) | |||
img = deskew(img) |
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.
Instead of img[0]
and img[1]
you can also destructure return parameters like so:
[output, angle] = deskew(img)
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.
Updated this with last commit, thanks for your input! 👍
Thanks for the PR @Dielee. Added some comments. 😊 |
I take a look at the PR as well. Thanks for the work you provided but I have serious concerns. The problem is not the pull request. There was a reason why I didn't add the auto rotation because it is non-trivial. I came across of variety of solution, to detect the rotation of the image (and the text). Sum brightnessesIdea:
Code
Thus, this worked astonishing bad. The intuition is that nearly white or grey pixels get close to black because the average of the pixels in the kernel is calculated every time. Therefore, this does not work as good as on images with high contrast. Using min rectangleIn short, the algorithm that is now in the After it calculates the angle but you might be noticed that not all angles are correctly identified. Since all images are taken in the landscape the current result is 3/6 (which is unacceptable. Therefore, I suggest closing the PR. In short, because the current detection is not safe and since most of the user takes an image in landscape (for large objects). As discusses, maybe we should add an angle tag in the config.yml. Regards, William |
Sure you can close this PR. It's maybe not trivial. |
Oh good work @Dielee. Can you maybe provide an new PR? Regards, |
Only with commit 1051c4d ? |
Yes, please. |
Done! |
No description provided.