Closed danieledapo closed 4 years ago
the CI failed, but I think it has nothing to do with the changes
Thank you for looking into this! I've added two comments.
The RNG changes look great.
I restarted the failed build on Windows.
Also, I'd like to add a proptest to test this feature because I think it would be a good way to test it. The test would be something like "generate a set of fixed colors and a small n, run the simulated annealing process, check that the fixed colors have not been modified". I'm not sure though if it's ok to add a dependency on proptest.
Coming from a Haskell background, I love property-based testing. I think it would have a place in the main pastel
library. For this particular case, I think we could also do without it (with a fixed set of colors and a fixed n).
Coming from a Haskell background, I love property-based testing. I think it would have a place in the main
pastel
library. For this particular case, I think we could also do without it (with a fixed set of colors and a fixed n).
We can do without for sure, but I'm pretty sure it would have caught the case of the two colors of which one is fixed. Anyway, I've added some tests that hopefully are good enough.
I had a chance to look at this again. I think it doesn't quite work the way I would expect it to. If you call
pastel distinct N col1 col2
with N > 2
and col1
and col2
two colors with a relatively close distance d_12, the algorithm will create new colors whose mutual distance is equal to d_12, but not necessarily much larger (as it will not optimize the distance further once it reaches d_12).
Let me explain in more detail...
I added a debugging feature that prints the distance matrix for the final set of colors (see #94). For the normal mode, it looks like this:
pastel distinct -v -m CIEDE2000 11
For N = 4
and CIEDE2000, it looks like this:
pastel distinct -v -m CIEDE2000 4
the minimal distance is 66.54.
Now let's take #3eb8ff
from this set and fix it. Let's also fix a second color which is quite close to it. Say #3eb8dd
. The new result looks like this:
pastel distinct -v -m CIEDE2000 4 3eb8ff 3eb8dd
Now obviously, we have a very low D(#3eb8ff
, #3eb8dd
) = 8.89. That is expected. But we also have another color in the set (#8882fb
) which did not get shifted very far from the two fixed colors (by 24.26 and 26.28). This is not really expected. Ideally, the newly generated colors would have distances of 66 (like above) or even higher.
Edit: Here is another example run that shows that the two new colors can also be quite close together:
I think this could be fixed by not considering distances between fixed colors when computing D_min
and D_mean
.
Sorry if I was not present these days, but I've been quite busy :sweat_smile:
Anyway, you're right that including the fixed colors in the score metrics was throwing off the algorithm. I've updated the code to not consider pairs of fixed colors for the metrics and I see that the final scores improved.
Do you think there are other problems with these changes?
Thank you very much!
With this PR the
distinct
command supports generating distinct colors of which some of them are predefined and cannot be changed.I'm not sure about what to do in cases of duplicate input colors like
pastel distinct 8 red red
. As of now, there will be two instances of the same color. I don't know what should be the expected behaviour here.I also took the chance of refactoring a bit how the simulated annealing process was generating random numbers. In particular, I've added an explicit rng as part of the
SimulatedAnnealing
struct and all the randomly generated numbers should pass through it now. The reason behind this refactoring was to be able to write deterministic unit tests which I think are feasible to do now. Incidentally, this also allows to experiment with different kinds of RNGs. For example, it might be interesting to try to swap the default cryptographically securethread_rng()
with a non secure RNG which should be quite a bit faster. Anyway, if you feel like these changes are not part of this pr (which is true) I'm happy create an issue or pull request where we can discuss this.Also, I'd like to add a proptest to test this feature because I think it would be a good way to test it. The test would be something like "generate a set of fixed colors and a small n, run the simulated annealing process, check that the fixed colors have not been modified". I'm not sure though if it's ok to add a dependency on proptest.