-
Notifications
You must be signed in to change notification settings - Fork 70
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
Refactor mutation probabilities #140
Conversation
a508698
to
ffef9ec
Compare
ffef9ec
to
81ebe67
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. It is nice to have a struct for the mutation probabilities instead of having to keep track of the ordering in the vector. The selection of mutation operator is now also much easier to see compared to the cumulative sum approach. I just left some comments.
I feel like |
Thanks for the review! |
Right now one specifies the mutation probabilities by passing in a long vector of floats. This is super uninterpretable, so this change fixes this. This will also be helpful for adding new mutations, like for the
shared-node-utils
branch.With this change, there is now a
MutationWeights
struct which you can specify as, e.g.,Where these numbers are the relative probability of each mutation (will be normalized afterward). This also makes the code more readable. Now, instead of doing a cumulative sum on the weights manually, and comparing to
rand()
, a method has been added of typerand(::MutationWeights)
. This will return a Symbol specifying the mutation chosen. For example:Right now, the parameter is passed in as
mutationWeights
rather thanmutation_weights
. I'm planning to refactor all of those in the future to be consistentlysnake_case
at some point, but not now.@johanbluecreek or @CharFox1 would one of you be willing to take a look?
Thanks!
Miles