fix: minimize total color distance in build_unique_palette#139
Conversation
Fixes part of inkstitch/inkstitch#2668 which references several problems. The main issues I resolved is inkstitch/inkstitch#1286 and inkstitch/inkstitch#1761. I'm positive it does NOT resolve inkstitch/inkstitch#2612 which I may try tackling in a future PR. **Problem**: when users exported to a .pes file, colors appeared to be swapped. **Root cause**: when converting the user's chosen threads to brother-compatible colors, `build_unique_palette` was greedily assigning palette colors which meant colors later in the threadlist could end up with a worse color than it should have. For example, let's say you want to stitch two threads in this order: `cream --> white` and your machine only has these threads available: `{black,snow}`. Since cream was evaluated first, it would be assigned to snow since that was the closest match, then white would be assigned to black since snow was already taken, resulting in `[snow,black]` when the better solution would have been `[black, snow]` since that pairing minimizes the total color distance across both threads. This is a well-known combinatorics problem: https://en.wikipedia.org/wiki/Assignment_problem Specifically it is of the "unbalanced" flavor since the number of available threads != the number of threads the user is using. **Solution**: use the hungarian algorithm to minimize the combined color shift. This is a rather nasty-looking function but I've purposely kept it generic to help show it is just a standard algorithm. An alternative is to use scipy.optimize.linear_sum_assignment (which itself is just implementing hungarian as well), but pystitch currently has no dependencies and I did not want to introduce scipy just for that. You might think another solution is to simply test every combination, but that would be O(n!) and even with our low numbers like n=64, that is catastrophic! I was trying to avoid adding complexity if there was a simpler solution, but I can't think of any other options. I did corner off the generic algorithm in its own file with some links to explanations, and I commentated on the updated `build_unique_palette` to help explain how it works.
| def build_unique_palette( | ||
| thread_palette: list[Optional["EmbThread"]], | ||
| threadlist: Sequence["EmbThread"], | ||
| ) -> list[Optional[int]]: |
There was a problem hiding this comment.
FWIW if the return type was actually a map<threadlist_stitch, palette_stitch> (i.e. a mapping from your desired threads to the available threads), I think some of the logic below would be easier to understand. Right now the return is a list of palette indices which makes it a bit difficult to mentally walk through the algorithm IMO.
But changing the return type makes this function differ from others in the file and requires upstream changes, so I did not do it.
| for i in range(1, len(palette)): | ||
| self.assertNotEqual(palette[i-1], palette[i]) | ||
|
|
||
| def test_unique_palette_order_invariant(self): |
There was a problem hiding this comment.
this test and the similar one in the new pec test file are the ones that fail without the src code changes, while the other two tests verifying thread-repeats reuse the same palette thread was just a test gap i wanted to fill before starting to mess with the function -- those two pass before and after src code changes.
| # Build a cost matrix for the minimal_assignment algorithm | ||
| cost = [ | ||
| [ | ||
| color_distance_red_mean( |
There was a problem hiding this comment.
While this PR fixes the order-invariant problem, we are still relying on the same color-comparison calculation: redmean -- which is fairly good. In inkstitch/inkstitch#568 tyrosinase mentions
Color conversions are always weird; I spent some time in the print shop world in the early days of desktop publishing, and RGB<->CMYK conversions have gotten some better but not a lot. It's a multidimensional space, and colors that are mathematically close aren't always close to the human eye. Grays are particularly subject to this: something can be close mathematically but not remotely "gray."
redmean is susceptible to this. If you'd like, I can draw up a comparison of what it would look like if we used a different algorithm, namely a ΔE equation. In fact, inkstitch is already using one for its catalog code, which could lead to some confusion for the user when that related functionality behaves differently than the pes export.
In any case, it'd be a separate PR.
| # trips C901 there. The branching is inherent to the shortest-augmenting-path | ||
| # formulation of the algorithm, not something worth splitting up just to | ||
| # satisfy the linter. | ||
| def minimal_assignment(cost: Sequence[Sequence[float]]) -> list[int]: # noqa: C901 |
There was a problem hiding this comment.
If preferred, I can open an alternative PR that adds uv and pulls in scipy, then use scipy.optimize.linear_sum_assignment instead of having our own hungarian algorithm.
If we do keep this function, I can add in a test that mimics one of the examples seen in the wiki: https://en.wikipedia.org/wiki/Hungarian_algorithm (the Alice/Bob/Carol worker problem).
1 participant
Fixes part of inkstitch/inkstitch#2668 which references several problems. The main issues I resolved is inkstitch/inkstitch#1286 and inkstitch/inkstitch#1761. I'm positive it does NOT resolve inkstitch/inkstitch#2612
which I may try tackling in a future PR.Problem: when users exported to a .pes file, colors appeared to be swapped.
Root cause: when converting the user's chosen threads to brother-compatible colors,
build_unique_palettewas greedily assigning palette colors which meant colors later in the threadlist could end up with a worse color than it should have.For example, let's say you want to stitch two threads in this order:
cream --> whiteand your machine only has these threads available:{black,snow}. Since cream was evaluated first, it would be assigned to snow since that was the closest match, then white would be assigned to black since snow was already taken, resulting in[snow,black]when the better solution would have been[black, snow]since that pairing minimizes the total color distance across both threads.This is a well-known combinatorics problem: https://en.wikipedia.org/wiki/Assignment_problem
Specifically it is of the "unbalanced" flavor since the number of available threads != the number of threads the user is using.
Solution: use the hungarian algorithm to minimize the combined color shift. This is a rather nasty-looking function but I've purposely kept it generic to help show it is just a standard algorithm. An alternative is to use scipy.optimize.linear_sum_assignment (which itself is just implementing hungarian as well), but pystitch currently has no dependencies and I did not want to introduce scipy just for that. You might think another solution is to simply test every combination, but that would be O(n!) and even with our low numbers like n=64, that is catastrophic!
I was trying to avoid adding complexity if there was a simpler solution, but I can't think of any other options. I did corner off the generic algorithm in its own file with some links to explanations, and I commentated on the updated
build_unique_paletteto help explain how it works.My design:
Using old version, outputting .pes then importing into Inkscape, note the pink and white swapped:
And now the with my PR's changes:
For reference, the PDF export, note that carnation came before white, thus reproducing the bug:
