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

better error message for breed #74

Open
koaning opened this issue Mar 18, 2018 · 7 comments
Open

better error message for breed #74

koaning opened this issue Mar 18, 2018 · 7 comments

Comments

@koaning
Copy link
Contributor

koaning commented Mar 18, 2018

Suppose that we are working with this bit of code.

from evol import Population, Evolution
from evol.logger import BaseLogger, SummaryLogger
from evol.helpers.pickers import pick_random

def change(gene, size=8):
  return min(BOARD_SIZE, max(1, gene + randint(-1, 1)))

def mutate_coords(chrmsm, p=0.1):
  return [(change(x), change(y)) if random() < p else (x,y) for x,y in chrmsm]

def run_evolution(range_num, evolve_num):
  chromosomes = [random_places() for i in range(POP_SIZE)]
  
  pop = Population(chromosomes=chromosomes, 
                   eval_function=score_tower)
  evo = (Evolution()
       .survive(fraction=0.2)
       .breed(pick_random, mutate_coords, p=0.2)
       .evaluate())

Can you find the error? It gives an error.

python towers.py 2 2
Traceback (most recent call last):
  File "/home/ec2-user/environment/evol-experiments/venv/local/lib/python3.6/dist-packages/evol/helpers/utils.py", line 52, in result
    return func(*args, **kwargs)
TypeError: mutate_coords() got multiple values for argument 'p'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "towers.py", line 45, in <module>
    fire.Fire(run_evolution)
  File "/home/ec2-user/environment/evol-experiments/venv/local/lib/python3.6/dist-packages/fire/core.py", line 127, in Fire
    component_trace = _Fire(component, args, context, name)
  File "/home/ec2-user/environment/evol-experiments/venv/local/lib/python3.6/dist-packages/fire/core.py", line 366, in _Fire
    component, remaining_args)
  File "/home/ec2-user/environment/evol-experiments/venv/local/lib/python3.6/dist-packages/fire/core.py", line 542, in _CallCallable
    result = fn(*varargs, **kwargs)
  File "towers.py", line 42, in run_evolution
    pop = pop.evolve(evo, n=evolve_num)
  File "/home/ec2-user/environment/evol-experiments/venv/local/lib/python3.6/dist-packages/evol/population/base.py", line 71, in evolve
    step.apply(result)
  File "/home/ec2-user/environment/evol-experiments/venv/local/lib/python3.6/dist-packages/evol/step.py", line 68, in apply
    return population.breed(**self.kwargs)
  File "/home/ec2-user/environment/evol-experiments/venv/local/lib/python3.6/dist-packages/evol/population/base.py", line 191, in breed
    self.individuals.append(Individual(chromosome=combiner(*chromosomes, **kwargs)))
  File "/home/ec2-user/environment/evol-experiments/venv/local/lib/python3.6/dist-packages/evol/helpers/utils.py", line 54, in result
    return func(*args, **{k: v for k, v in kwargs.items() if k in signature(func).parameters})
TypeError: mutate_coords() got multiple values for argument 'p'

The user forgot to specify the number of parents in the parent picker. This is something that could go wrong rather often and it is something that needs a better error message. Something in order of "your parent picker outputs 2 parents but your parent merger only assumes one".

@rogiervandergeer
Copy link
Collaborator

One way to solve this is to provide the chromosomes as a list/tuple to a single argument of the combiner. Then it is the user's responsibility to extract the chromosomes from the list.

@koaning
Copy link
Contributor Author

koaning commented Mar 20, 2018

its a breaking api change though. it might help make many things much clearer though.

@rogiervandergeer
Copy link
Collaborator

A downside of passing the parents in a sequence instead of multiple arguments is that lambda combiner functions lose elegance; e.g. when using a chromosome class with a combine method it goes from:

pop.breed(..., combiner=lambda mom, dad: mom.combine(dad))

to

pop.breed(..., combiner=lambda parents: parents[0].combine(parents[1]))

@rogiervandergeer
Copy link
Collaborator

Found bug #76 while playing with this problem

@rogiervandergeer
Copy link
Collaborator

One can try to solve this problem by making the non-chromosome arguments keyword only arguments. E.g., a function defined as

def f(x, y, *, z):
    return 0

can not be called as f(1, 2, 3) but must be called as f(1, 2, z=3). Of course we can not force our users to define combiner functions like that, but we can do this with our own combiners.

Forcing our users to do this would require that we can detect whether or not an argument is supposed to be a chromosome. Since we do not know the type of a chromosome (it could be anything from a float to an object) we cannot rely on annotation (type hints) of the arguments, let alone that we cannot rely on users properly annotating the arguments to their functions. Also, we cannot rely on the extra arguments to have a default value - as they may not have that by design - or the chromosome arguments to have no default value (e.g. a function like combine(mom, dad=None) that allows mom to asexually reproduce is viable).

Hence passing the chromosomes in a single argument is still the only solution that will catch every exception.

So do we want this to be completely fool proof, or can we live with making our own combiners have keyword-only arguments and possible augmenting the message of exceptions that are raised in the offspring_generator?

@koaning
Copy link
Contributor Author

koaning commented Mar 23, 2018

I think making it fool proof is better. A single rule is better than many and after having thought about it, i like the idea that chromosomes will always be an iterable ... preferably a tuple maybe ... but definitely just a single variable. It makes everything less suprising and much easier to provide useful errors.

The main downside is that this change will break a lot of things and is a breaking API change. Technically, this means that changing this might make evol v1.0.0.

@rogiervandergeer
Copy link
Collaborator

See #78

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

No branches or pull requests

2 participants