search

goncalopp / simple-ocr-opencv

A simple python OCR engine using opencv
GNU Affero General Public License v3.0
524 stars 175 forks source link

Replace ImageFile with Image class, OO image representation in memory, pillow compatibility improvements #26

Closed RedFantom closed 6 years ago

RedFantom commented 7 years ago

These changes replace the ImageFile class with the Image class, allowing working with image files in memory, compatibility with Pillow Images, and providing a more clear interface to work with overall.

Update: The branch was rebased onto master to simplify comparison and merging.

RedFantom commented 7 years ago

If we actually do choose to create an Image (Buffer) class and an ImageFile class, then I think it would be nice to include the option to swap out the images stored inside them as well? For the Image by reading a new image array and for the ImageFile by reading a new image file (and then of course removing the ground data).

goncalopp commented 7 years ago

Is there an advantage to doing img.reload() vs ImageFile.from_file(img.path)?

In general, I tend to favour keeping data structures immutable (by contract), as it makes them easier to work with. But if there's a strong use case it should be fine

RedFantom commented 7 years ago

Oh, I completely forgot that such changes would indeed make the class to lose immutability, and the use case I had in mind (which could even be made obsolete with a few more lines of code on the user's part), is not nearly important enough to lose immutability (especially not the thread-safe part).

goncalopp commented 7 years ago

Hi RedFantom, Sorry, it's been a long time since I looked into this, and I lost a bit of context on it. I'll probably have to re-review the whole thing. Should I take a look at the code again, or are you still making changes?

RedFantom commented 7 years ago

@goncalopp The same goes for me. If I look at the review you left earlier (long ago), it looks like I should remove the debug output. And honestly, that actually makes a lot of sense. Apart from that, I think I had already covered all the other stuff... So, I'll push one more commit and then I think you can go ahead an re-review.

RedFantom commented 7 years ago

Sorry for making a mess of the commits 😞

goncalopp commented 6 years ago

Hi RedFantom, It's been a while! I found some time and energy to re-review this, and it's looking great :)

I did a couple of cosmetic changes, and fixed a error that was making tests fail, can you please take a look at these two commits? (on top of your branch) cc50fdf105d9d2f0ac5962822446466f5cfedb0b ffcaa8ce950f9f3219944cdff6d967319d546cbc

If you agree with them and merge them in your branch, I'm happy to approve this PR

RedFantom commented 6 years ago

It has most certainly been a long time! Thank you for taking a look at it now, better late than never, right? :smiley:

I have cherry-picked your commits, they were looking good! For overview reasons, I really think this branch should be squash-merged.

goncalopp commented 6 years ago

Yup, sorry for the delay! I've been squash merging all the PRs, it's better for both shorter commit history and tracking down bugs with git bisect

Thank you for all your work, this patch really takes the project to a new level :)

RedFantom commented 6 years ago

It has has been my pleasure. You have helped me learn a lot about maintaining a library and how to write better code :) .