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

Merge contributions from RedFantom's fork #14

Closed goncalopp closed 7 years ago

goncalopp commented 7 years ago

Continued discussion from https://github.com/goncalopp/simple-ocr-opencv/issues/13

Fork at https://github.com/RedFantom/simple-ocr-opencv

@RedFantom A PR sounds great, but please make sure its in a reviewable state - i.e.: formatting is separated from contributions, and ideally a small (squashed) number of commits. If you don't have the time right now to squash/rebase everything I'll try and have a go at replicating your changes incrementally.

Thanks!

RedFantom commented 7 years ago

@goncalopp Most of the commits contain additions for unittests, and I wrote an Improver-class that uses PIL to complement the already available ImageProcessor classes. I also wrote a SimpleOCR class that does the object creation and such so the code becomes easier to use, it's not much more than a wrapper class. I wanted to do more complicated changes, but thus far I haven't really had time. With formatting separated from contributions, do you mean that the PEP-8 updates should be separated from the actual code I wrote? I'm not sure how to do that...

zuphilip commented 7 years ago

It should be possible to create a PR with a lot of individual commits and review the changes as whole. E.g. here is the diff of the two branches (ignoring inline spaces changes): https://github.com/goncalopp/simple-ocr-opencv/compare/master...RedFantom:master?w=1#files_bucket . The same diff (with or without the space changes) you will see in a PR.

goncalopp commented 7 years ago

@RedFantom Thanks!

With formatting separated from contributions, do you mean that the PEP-8 updates should be separated from the actual code I wrote? I'm not sure how to do that... Yes. Don't worry about it, I'll try to recreate your changes, and let you know once I have it cleanly separated You might want to check [this page]https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History), rewriting commits is quite useful sometimes

@zuphilip It's certainly possible, we did that a few weeks back. The problem is that the outcome is binary (merge / no merge), and as you add more and more things, it's more likely there'll be things that shouldn't be merged. So multiple granular PRs saves everyone time in the end

RedFantom commented 7 years ago

@goncalopp Thank you for linking the article, I had no idea that was even possible! Reading it was very enlightening. If you are already trying to separate the changes, I guess it's better not to interfere. I hope I made my commit messages clear enough, and if you have any questions, please ask!

goncalopp commented 7 years ago

Ok, I spent one hour trying to merge this today, and it's incredibly tricky. I'm using autopep8 to format the code, but it looks like the tool you used to format it does many things differently (particularly docstrings), so there's tons of conflicts

I think the easiest way to go around this is probably going back to the original commit that you based your fork on (8eeda3730), have your tool apply ONLY the formatting to all the files, commit that on a separate branch, then rebase your master branch on top of that. Can you try to give it a go? Let me know if you need any help

RedFantom commented 7 years ago

@goncalopp Well, I'm currently trying to separate the changes I made into four categories: added tests (branch tests), formatting (branch pep8), changes to license and readme files (branch readme) and added features, which I'm starting on now. Then the changes will be separated, and then I can create multiple PRs (one for each branch), right?

BTW: I used PyCharm's built-in PEP-8 checks to restyle the code. The keyboard combo Ctrl+Alt+L reformats the whitespaces, and then I did every docstring by hand to follow the PEP-8 style to the letter.

Edit: Sorry about the PR earlier, it was not my intention to open one yet, just to view the diff :-{ . Edit 2: What about the OpenCV 3 compatibility? OpenCV 2 is pretty much obsolete now, right?

goncalopp commented 7 years ago

@RedFantom Thanks!

Then the changes will be separated, and then I can create multiple PRs (one for each branch), right?

Yes, that sounds great! I think we should start with the pep8 one, to make our lives easier. I'll take a look

I did check PyCharm before, but its not on the repositories and installation seems quite painful, unfortunately.

Edit: Sorry about the PR earlier, it was not my intention to open one yet, just to view the diff :-{ .

No worries. I think you should have a "Compare" button on your branch page - at least I see it when I visit your branches. Of course, if you don't mind using the command line git diff -w is particularly helpful for formatting changes

What about the OpenCV 3 compatibility? OpenCV 2 is pretty much obsolete now, right?

OpenCV 2.3 is the default on debian stable (which I personally use), so it's far from obsolete

RedFantom commented 7 years ago

@goncalopp Installation of PyCharm is actually rather easy. I'm running it on Ubuntu 16.04 LTS on one of the machines I work on. You can download an archive version of PyCharm from the site for Linux and then you can use a .sh file to start it. Everything you need is included.

OpenCV 2 is the default, even for the newest Debian version? That significantly complicates things! OpenCV 2 is a pain (a real bad pain) to install on Windows, while OpenCV 3 is done in a snap. This also goes for Ubuntu 16.04 LTS... They are very much incompatible though.

Edit: So, you need to code to run on OpenCV 2 for Debian Stable, and I cannot use OpenCV 2 on my systems because it's nearly impossible to install on Windows and Ubuntu 16.04 only provides OpenCV 3. That's quite a problem. I had never really expected my changes to have to be merged back into the original repository, to be honest, so I applied the formatting changes after merging opencv3 into the master branch of my fork.

RedFantom commented 7 years ago

Next up is Python 3 support, I guess? Currently, I've got a branch python3 for Python 3 compatibility. I ran the 2to3 utility included with Python and made some more changes, but there appears to be some kind of encoding issue, I think? It is the same as I encountered before on my first attempt with a lot more changes, namely with the 1 changing place. I'm still searching for a cause.

If the code would be put into a Python package, I think that would make it more accessible. I've made a draft of how it would look like in the distutils branch, but the tests will have to be changed accordingly. I'll try to work on improving the documentation further soon, but I won't have much time for the upcoming three weeks, unfortunately.

goncalopp commented 7 years ago

Your python3 branch's tests pass for me on python 2. How are you actually testing on python3? Does your distro package python3-opencv? Or are you installing it manually?

Thanks for the packaging! I'm happy with the name simpleocr, but we should probably not use version 1.0.0 until we're done messing with it :) I might cherry-pick your changes, or feel free to submit a PR if you'd like.

RedFantom commented 7 years ago

Yes, the tests pass just fine under Python 2, but this is not the case for Python 3. I'm trying to test the Python 3 version on Windows 7 64-bit with Python 3.6 64-bit with the numpy and opencv-python PyPI packages. Also, I have attempted to test the changes on Travis-CI, but the Python 3 environments fail to activate. It appears that it is a problem within Travis-CI.

The packaging is not completely done actually, as the tests on Travis-CI still fail because of path issues with the data folder. I will submit a PR if it's ready :) .

goncalopp commented 7 years ago

I don't think python3 on Travis would work at all with the current config, even if it didn't get stuck on getting python3. apt-get install python-opencv only installs opencv bindings for python2, not python3. I'll try to take a look at it when I have a bit of time

RedFantom commented 7 years ago

So, I had some time again today (finally!), and I looked into the whole Python 3 compatibility further. The only problem is the OpenCV installation, it seems. When installing OpenCV-Python through pip on Python 3, it does work, but surprisingly, different results are returned for various functions. I noticed this when moving my GSF Parser code base to Python 3, as it uses template matching. It is very possible that the problem is not in the code of this project, but simply in the wheels provided by pip to install OpenCV Python on Windows!

The results returned when template matching are consistent, though consistently incorrect, both in comparison to Python 2 and manual matching. And they're not just a bit off either, they're way off.

The results when testing this project is that there are 1's all over the place: there are too many in the result, and in the wrong places. I will look into the situation on Linux, but it appears this is a complicated issue...

RedFantom commented 7 years ago

Today I had my Linux system up and running, and decided to install OpenCV 3 for Python 3. Unfortunately, I didn't have any luck when compiling my own version, even when renaming the resulting shared objects manually it wouldn't import cv2. A wheel package is provided for Linux through pip though, but that has the same problem as on Windows. Do you think it's time to get into looking into issues of OpenCV itself, and when necessary open an issue report? @goncalopp

goncalopp commented 7 years ago

So this happens on Linux as well? I'll try to take a look then. I would expect it's a issue on our side, not OpenCV. Is there anything in particular needed to reproduce it? Just running the existing unit tests?

RedFantom commented 7 years ago

@goncalopp Yes, I thought so at first as well, but unfortunately the results for Linux and Windows are the same with OpenCV 3 and Python 3 if the package from PyPI is used. The only thing I haven't successfully tried yet is compiling from source and then installing it, but my suspicion is only strengthened by the fact that I'm experiencing similar problems in my GSF Parser in the vision3 branch, where I combine Python 3 and OpenCV 3. There the template matching provides incorrect results. This makes me think that at least the PyPI packages have some problems with interaction between the C library and the Python bindings or sometehing.

If you would like to reproduce the issue as I have experienced it, all you have to do is use python3 -m pip install opencv-python nose and then run the unit tests. The error will be in the test_ocr.py, with a difference in arrays. This is the same for Linux and Windows, both tested on Python 3.5. The results are fine with Python 2.7 on both platforms (as my testing on Windows and the Travis-CI of this repository proves).

goncalopp commented 7 years ago

Ok, I can reproduce it. Out of curiosity, are you able to run example.py? I'm getting an error on cv2.imshow, about missing libraries, it seems like the library is being installed pre-compiled with pip?

RedFantom commented 7 years ago

@goncalopp The library OpenCV is actually written in C++, and by using pip you install a wheel-archive, with a pre-compiled .so file on Linux and .dll file on Windows suitable for your system and Python installation. I do indeed get the following error when running example.py:

cv2.error: /io/opencv/modules/highgui/src/window.cpp:583: error: (-2) The function is not implemented. Rebuild the library with Windows, GTK+ 2.x or Carbon support. If you are on Ubuntu or Debian, install libgtk2.0-dev and pkg-config, then re-run cmake or configure script in the function cvShowImage

I did install libgtk2.0-dev when compiling my own OpenCV, but it didn't install correctly, so I used the PyPI one instead. I will try again and test again afterwards, as it's looking like the PyPI version of opencv-python is not compiled with all the options.

Update: I was able to install OpenCV 3.2 correctly under Python 3 on Kubuntu in my virtual machine (cmake took a while tough), but it gave the same result, except that it did not have a problem with cv2.imshow because libgtk2.0-dev was installed.

goncalopp commented 7 years ago

Actually, reading the package description on PyPi, they make it clear:

Which makes it quite useless - nothing related to UI will work, and it's really hard to diagnose anything without seeing what's happening I was also trying this on Ubuntu 17.04, but the opencv package on the repository seems to be version 2.4, so that doesn't help either...

I'm not sure it's going to be easy to support cv3. Does the UI work on windows?

RedFantom commented 7 years ago

@goncalopp It does, actually, because the support for cv2.imshow() is automatically built on Windows. OpenCV 3 is already supported, don't you mean Python 3?

Even if I compile my own opencv-python on Ubuntu, I still get the wrong results (I did compile it with GTK 2.0+ support), so it must be a deeper issue than just missing dependencies.

goncalopp commented 7 years ago

@RedFantom I took a look at this today and discovered what the problem is. Classification is, in fact, working correctly. As is OpenCV.

It took me over an hour, but here's the bug, in segmentation_aux.py: order = mlw * (s[:, 1] / mlh) + s[:, 0] / is meant to be integer division. In python 3, this needs to become //. Since the segment order is different, the returned characters are in the wrong order as well.

That's it :) After changing this, all tests pass

RedFantom commented 7 years ago

@goncalopp Awesome! It works now indeed :) . Thank you for figuring it out! If #23 gets merged, then my only other goal was to package the project with distutils like I had already given a shot in the RedFantom/distutils branch, and then optionally upload to PyPi. I'll try to finish the packaging in the branch today.

Update: Now that I think about it, this still doesn't explain why the template matching results are different. I guess that'll be something completely unrelated then.

goncalopp commented 7 years ago

@RedFantom I think all the original changes from your fork are in? Can we close this?

RedFantom commented 7 years ago

Yes, they are indeed all in now. Thank you for merging them all 😄

goncalopp commented 7 years ago

Thank you for taking the time to contributing the changes!