Open Y1fanHE opened 3 years ago
Thanks for taking the time to implement this feature! I appreciate you dedicating your time to this project. I think your design is great. I only have a couple of suggestions.
I think it would be helpful to use a more specific name than fix
. Perhaps limit
or truncate
. Also the "fix" method doesn't seem like it should be called outside of the VariationOperator
class and sub-classes so it should probably be made private (ie. _fix
, _limit
, _truncate
, or whatever.)
I'm not sure if there needs to be a separate method for returning fixed genomes. To address this I would would recommend the doing the following:
produce
method to _produce
in VariationOperator
and all of its sub-classes. produce_and_fix
to produce
(that method will only be on the base class).max_genome_size
argument of the new produce
to max_genome_size: Optional[int] = None
.produce
method call produce
and truncate the child genome if max_genome_size
is not None
.This way the implementations of GeneticAlgorithm
and SimulatedAnnealing
can always call produce
regardless if a max genome size is provided or not.
It looks like this PR contains the commits from your last PR. Those have already been squashed and merged into master. Can you drop them from this PR and update your branch to the latest version of master?
As an aside, it is often easiest to do the work for each PR on a separate branch. I recommend this guide.
I have made the update based on your comments and opened a new PR #161
I have tried to add
max_genome_size
parameter in the variation steps. This parameter can be specified in the estimator.I created a method
fix()
in theVariationOperator
class. Also a methodproduce_and_fix()
as follows.Inside the algorithms, you can use
produce_and_fix()
instead ofproduce()
. I am not sure whether you are happy with the implementation, tell me if you have any suggestion.