search

gtaylor / python-colormath

A python module that abstracts common color math operations. For example, converting from CIE L*a*b to XYZ, or from RGB to CMYK
python-colormath.readthedocs.org
BSD 3-Clause "New" or "Revised" License
456 stars 83 forks source link

Refactored color space conversion routine and added graph based algorithm #35

Closed MichaelMauderer closed 10 years ago

MichaelMauderer commented 10 years ago

This is a proposal for a simplification of the current way type conversions between color spaces are handled.

The current implementation has the problem, that for every new color space a lot of manual work is required as new conversions between all available color spaces need to be added.

My proposal would be to substitute this approach of manually defining paths by a registration approach where a conversion function is centrally registered (e.g., using a decorator) and conversion paths are algorithmically determined based on this information.

This is a potential implementation of this approach:

  1. I added a decorator based registration mechanism for color space conversion functions that annotates them with start and target color space (e.g., @color_conversion_function(LuvColor, XYZColor)
  2. I re-factored the current way a path between color spaces is determined so a ColorConversionManager is used instead of a global function, allowing easy swapping of implementations. (I moved the old implementation into the CuratedConversionManager)
  3. I added a NetworkX based GraphConversionManager that builds a directed Graph of conversions and then can easily determine the shortest path between two color spaces.

Currently the old implementation is still present and used as a fall-back in case NetworkX can not be imported.

I am happy to take any comments.

MichaelMauderer commented 10 years ago

Removing the old conversion table is just a matter of deleting the code. Right now it is only there as fall-back so things don't (yet) stop working without the new dependency.

It might be a good idea to add some unit tests that look at the functionality of the ColorConversionManager detached from the rest of the system. Right now I relied on the existing color conversion tests to make sure things are working.

gtaylor commented 10 years ago

I bumped the version number in git master in preparation for this.

gtaylor commented 10 years ago

Looks like the merges may have created some conflicts. I'm going to try to wade through this soon and make some of the changes we talked about (removing the old conversion code, etc).

MichaelMauderer commented 10 years ago

Okay. If it helps, I can rebase the branch.

I also already removed the old code in 38db371

gtaylor commented 10 years ago

I completely missed that. Whoops!

If you want to rebase, go on ahead and merge this into master and we'll continue to poke at it for a while before doing our big 2.1.0 release.