search

esa / pygmo2

A Python platform to perform parallel computations of optimisation tasks (global and local) via the asynchronous generalized island model.
https://esa.github.io/pygmo2/
Mozilla Public License 2.0
442 stars 56 forks source link

Misleading error message? #50

Open darioizzo opened 4 years ago

darioizzo commented 4 years ago

I believe here and elwhere in the pop constructor the kwarg size need renaming to pop_size

https://github.com/esa/pygmo2/blob/be6c148b829f30bebf800cbc0dc4d78c1938f7c2/pygmo/__init__.py#L362

bluescarni commented 4 years ago

I believe that you are referring to the fact that in the archipelago constructor the population size arg is called pop_size?

That choice was made because, in the archipelago ctor, just having size would be ambiguous with the number of islands (which is also the archipelago's "size"), thus it was renamed to pop_size there.

I don't think it's necessary to change the name of the size argument in the island/population ctors because I don't think there's any risk of ambiguity in those cases. I think it would also be inconsistent with the other arguments (why not pop_prob, pop_seed, and so on).

darioizzo commented 4 years ago

Now I understood the confusion. When you try to build an archipelago with:

archi = pygmo.archipelago(algo = pygmo.de(100), prob = pygmo.ackley(10), n= 20)

you get the error:

"unable to construct a population from the arguments of the island constructor: you must either pass a population ('pop') or a set of arguments that can be used to build one ('prob', 'size' and, optionally, 'seed' and 'b')"

which is correct but a bit misleading as the next try I would do is to add size to he archi constructor as hinted in the text (as the text refers to the island constructor, which is invoked behind the scenes).

So in this sense renaming all to pop_size would be more consistent. But I also understand your argumant. And technically the current situation, albeit a bit lawerish, is correctly documented.

bluescarni commented 4 years ago

Ok yeah, that's misleading.

I don't have a good solution at the moment. One of the nice things of the archi constructor is that it forwards most arguments to the island/pop constructors, and I'd like not do lose that because it allows for a very clean implementation. Perhaps we should have adopted another kwarg name for the number of islands in the archipelago instead, but I feel like whatever we end up doing, it will break backwards compatibility.

If we do backwards incompatible changes, we should at least try to make them all in one go to minimize pain, so perhaps it would be time to discuss uniforming the kwargs of the core classes (I think we had an issue open at one point about this).