-
-
Notifications
You must be signed in to change notification settings - Fork 105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add fixed colors to distinct #88
Add fixed colors to distinct #88
Conversation
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. |
Coming from a Haskell background, I love property-based testing. I think it would have a place in the main |
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
with 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:
For
Now let's take
Now obviously, we have a very low D( 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 |
This allows to easily experiment with different RNG and to more easily test it given that we can now seed the rng as we like.
Previously the algorithm was not updating the closest_pair for cases where only a single color was fixed, but we can actually use that pair if we forcefully look at the non fixed one.
Sorry if I was not present these days, but I've been quite busy 😅 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! |
Released in v0.6.0. |
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.