Skip to content
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 max_genome_size parameter #160

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add max_genome_size parameter #160

wants to merge 6 commits into from

Conversation

Y1fanHE
Copy link
Contributor

@Y1fanHE Y1fanHE commented Oct 23, 2021

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 the VariationOperator class. Also a method produce_and_fix() as follows.

def fix(self, child: Genome, max_genome_size: int) -> Genome:
    if len(child) > max_genome_size:
        child = child[:max_genome_size]
        return child
def produce_and_fix(self, parents: Sequence[Genome], spawner: GeneSpawner, max_genome_size: int) -> Genome:
    return self.fix(self.produce(parents, spawner), max_genome_size)

Inside the algorithms, you can use produce_and_fix() instead of produce(). I am not sure whether you are happy with the implementation, tell me if you have any suggestion.

@erp12
Copy link
Owner

erp12 commented Oct 29, 2021

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.

  1. 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.)

  2. 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:

    1. Rename the current produce method to _produce in VariationOperator and all of its sub-classes.
    2. Rename produce_and_fix to produce (that method will only be on the base class).
    3. Change the max_genome_size argument of the new produce to max_genome_size: Optional[int] = None.
    4. In the new 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.

@Y1fanHE
Copy link
Contributor Author

Y1fanHE commented Nov 9, 2021

I have made the update based on your comments and opened a new PR #161

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants