Merge Improver and TerminalGrounder classes into master of original repo

[RedFantom]

These commits contain the added Improver and TerminalGrounder classes. The Improver class should allow more images to be segmented because it allows the modification of color saturation, sharpness, brightness and contrast with default values that helps to improve most images. This is to make the segmentation of images easier. Also added is the TerminalGrounder class, which allows the grounding of image files with an interactive shell. The image cannot be shown in the terminal, but the amount of segments is provided. The changes in this branch depend on PIL.

Note: the images in this branch were supposed to be used for additional testing of the Improver class, however, this is not yet implemented. Should they be kept or should they be kicked? @goncalopp

This PR is related to issue #14 . Squash merging this branch with a new commit message is recommended.

[goncalopp]

Yes, the images and travis.yml should be excluded for now.

What's the intended use for TerminalGrounder? Why not just use TextGrounder if you're on the terminal?

[RedFantom]

Well, if you know how many characters your image is supposed to have and you know that it is the same as the amount of segments found, you can also ground it interactively in the Terminal. I admit that the use-case is probably limited to only a very small percentage, but I thought it should be an option.

[RedFantom]

I'll try rebasing these commits soon. I hope to have time for it upcoming Friday, maybe Thursday, but certainly not earlier.

[goncalopp]

Should we do this one first? Or the unit tests?

[RedFantom]

This branch is now rebased successfully onto master, and in the process I cleaned up some more things that I apparently missed before, sorry :( . There are no tests available for them at the moment though, because the images that I added to test segmentation have been removed. I could re-add them and then add the unittests again though. Update: So it appears that in the rebasing process I have lost the TerminalGrounder. There were so many conflicts I missed it. I'll re-add it.

[RedFantom]

Sorry for putting the TerminalGrounder in the list on the wiki page already, I was a bit overenthusiastic -_-'.

[goncalopp]

Thanks, this is looking much better :)

I didn't fully understand why you added individual functions though, for what was a parameter before? I changed it a bit to work with default arguments, can you please take a look on 8362ef97605110ad65b4f143539357ecfcf17486 (untested) and let me know what you think?

Sorry for putting the TerminalGrounder in the list on the wiki page already, I was a bit overenthusiastic -_-'.

No worries at all, thanks for improving documentation!

[goncalopp]

I think the repetition of the image = ImageEnhance.X(image).enhance(X) could be improved by taking arguments as kwargs, but it would hurt readability a lot, so it's probably not worth it for reducing 5 lines or so

[RedFantom]

Perhaps making the default argument None could work as well, but I mainly added the individual functions so that the improvements can also be executed by the user in a different order than the one pre-programmed in enhance_image, because the order in which the functions are executed can have significant influence on the end result and I thought it may be best to allow the user to execute the functions in a custom order as well. I know it's not the most efficient way of implementing that, as in every step the user does two conversions that are not immediately required (to and from Pillow format). What would of course be possible is writing one function that takes a list of classes as an argument and loops through that, but I don't know whether that would be very readable. Perhaps if I add it to the documentation as well (I was planning to) that it would be feasible?

[goncalopp]

Unless I misunderstood your comment, you can still do exactly that with my edit:

enhance_image(imagefile, brightness=1.3)
enhance_image(imagefile, color=0.5)

That's why the default argument value (None) defaults to a noop - so you're not forced to do anything you don't want to. I think it should be equivalent to having multiple discrete functions?

The problem I see with taking a list a classes as the argument is that you're exposing implementation details (the fact that you're using PIL) to the caller, and expecting them to know what the correct classes are.

We could take a list of pairs of (parameter_name, parameter_value), or two lists of [*parameter_names] and [*parameter_values] but it would need more code and hurt readability and convenience for the caller. Like you mention, converting back-and-forth is not ideal, but I don't think performance will realistically ever become a issue for this kind of thing. If it ever does, let's worry about it then :)

[RedFantom]

Yes, that would be a perfectly viable option now, using the keyword arguments. I just thought it might not be clear enough, but I guess it is then! In that case I will cherry-pick your commit and push it here!

[goncalopp]

Alright, this is looking pretty good, thanks! :) I did a final pass over the code, there's only two minor issues

[RedFantom]

The tests pass on my Windows machine with OpenCV 3 as well.

[goncalopp]

Thanks! I went ahead to delete a bit more repeated code on the tests and add assertions to make sure it's currently (and will keep) doing something, and not merely not raising a exception Can you please take a look at e3f9a0f6a7e065729d022141c1c49945cc9babc4 and push it if you agree with the contents? Feel free to make further changes, it was a bit rushed

[RedFantom]

Looks good! My commit earlier was rushed as well, so I actually just thought of it last night that I should've removed more code repetition.