move semantics not rigorously thought-out

[rumschuettel]

Hi,
the following suggestions for pagmo:

1. Most constructors (e.g. pagmo::problem, pagmo::population etc.) use move semantics && to ingest its arguments. This leaves the references outside invalid (clearly), as the resources are swapped. E.g. pagmo::problem should then not give the user the possibility to still access the leftover problem, e.g. by turning it into a null_problem.
2. When passing e.g. pagmo::pso to pagmo::algorithm, gen is ignored in evolve.
3. m_fevals is not incremented in algorithm::evolve, as that function takes population by value, not by reference.

Generally your usage of move semantics and references is not very thorough, so it would be worth spending some effort making the api more consistent.

Best,
Johannes

[bluescarni]

Hello Johannes, thanks for the feedback.

I am not sure I understood completely your comments, so let me go through them, and please correct me if I misunderstood.

1. You are right that move construction generally leaves the object in a state which is only destructible and assignable (as stated in the documentation). You are also right that, instead, we could re-initialise the problem with a null_problem after a move operation. Is that what you meant? We didn't bother because we did not come up with a use case scenario that would justify the effort of doing so, as the only way of having an "unsafe" problem is by using explicitly std::move() (or, equivalently, an explicit cast to rvalue ref). And neither we nor our users so fare have run into problems in practice due to this, but if you feel like there's high potential of misuse of a moved-from problem/algo/etc. we could make the effort to strengthen the safety of a moved-from object as you suggest.

2. Are you reporting that pso is ignoring some parameters or is this a more general comment about the algorithm class? pagmo::algorithm per se does not know anything about generations etc.

3. algorithm::evolve() takes a population by const reference, but specific algorithms sometimes implement the method with a parameter by copy. population copies are in general unavoidable because the API must accommodate also uses in which it's not possible to share a pointer/ref to a population (e.g., when doing multiprocessing, running on clusters, etc. - in those cases even serialization is involved). It is thus up to the specific algorithms/problems/etc. to make sure that the returned population is built from the same problem object used for the objfun evaluations, otherwise the fevals counter will not be updated. Are you reporting that some algortihms are not returning populations with the fevals counter updated? In that case we would consider that a bug.

You are welcome to head over to https://gitter.im/pagmo2/Lobby if you want to have a discussion about this.

Cheers,

Francesco.

[rumschuettel]

1 and 3. I think my issue was partially confusion, partially that the same code in python and c++ can behave differently. For instance, querying the number of function evaluations in python can be done with the original problem, even when using population and algorithm directly.
2. I'll post some code on monday clarifying what I meant.

[rumschuettel]

Ok, here a concrete example of my issue. If I use "population" directly, it takes a reference to my problem (assuming I pass the constructor an lvalue, and I don't use std::move). algorithm::evolve, however, is defined such that it takes a const reference, meaning I have to override my population with the return value of evolve. This leads to behaviour that differs between pagmo and pygmo:

pagmo::problem prob{ my_prob() };
pagmo::algorithm algo{ pagmo::bee_colony(20u) };
pagmo::population pop{ prob, 10u };
alg.evolve(pop); 
cout << pop.champion_f(); // does not give the right candidate, as e.g. bee_colony took population by value, and returns by value the evolved copy

# pygmo: the following works
problem = pygmo.problem( my_prob() )
solver = pygmo.algorithm(pygmo.bee_colony(gen = 20))
population = pygmo.population(problem, 10)
print(-population.champion_f) // works flawlessly

I think this is by design and has to do with UDPs in python not being thread-safe, so it's ok to keep a reference to the original problem around. I found this very confusing though, and it might be worth specifying how the population/algorithm/whatnot are intended to be chained together.

[bluescarni]

@rumschuettel

We designed pagmo/pygmo's interface in a "functional" fashion, in the sense that usually we prefer functions that take an input parameter and return an output value, rather than modifying objects in-place. In particular, the evolve() method of the algorithm takes a population as input and returns a population as output. Thus, in your code example

pagmo::problem prob{ my_prob() };
pagmo::algorithm algo{ pagmo::bee_colony(20u) };
pagmo::population pop{ prob, 10u };
alg.evolve(pop); 
cout << pop.champion_f();

what we see is the expected behaviour: the evolved population is returned by the alg.evolve(pop) call (but it's not stored anywhere, so it is effectively lost), and what you are printing is the champion of the original population. You need to do something like this instead:

pagmo::population pop{ prob, 10u };
pop = alg.evolve(pop); 
cout << pop.champion_f();

This should print the champion of the evolved population. The behaviour should be the same in C++ and Python (otherwise it's a bug).

Does that clear things up?

[rumschuettel]

Jup, that's exactly right. I'm just saying that in python this behaviour is not enforced, i.e. my second example from above works fine, which makes it hard at first to wrap your head around coding the same stuff in c++. I doubt it's a bug but probably just something that has to do with how python manages memory(i.e. that the original problem is just an alias, and the copy never actually happens---which is fine, since python UDPs are not thread-safe anyway).

Another related "issue" (again, not a bug) is that if e.g. bee_colony takes population by value, there's a copy happening; now, since population contains the problem (can be move'd, great!), and population has a default copy constructor (i.e. deep-copy everything), one would assume that the problem is deep-copied too, which is terrible if the UDP is expensive to create or has a vast amount of allocated memory.

Just that this is not what is happening. If one checks the constructor of problem, you pass it through make_unique, which forwards it to a new T(...) call, so now the problem contains a pointer (!) to detail::prob_inner, which contains the UDP, and which implements move semantics; this solves the copying-of-resources issue if the UDP is passed by rvalue reference in first place, and indeed it cannot be passed as lvalue---without having to care about move semantics for the UDP oneself.

While I agree that this works (after digging through the code), it would be amazing if you could update your documentation that gives some information about your design decisions and guarantees when a copy is actually happening: the compiler error one gets because problem is not overloaded to accept an lvalue is useless, at best, and then one opens pandora's box and files a bug report which isn't actually one ;-).

Let me add one last comment in support of this vague complaint: problem and algorithm are supposed to take rvalue references to the UDP and UDA, respectively. population accepts problem as lvalue, though. It's not immediately obvious why that is: i.e. that one might want multiple populations with the same problem. This is especially confusing because the python interface knows nothing about rvalues or lvalues, so there---as usual---things "just work".

I'm happy to have this issue closed, as the "bug" was more my confusion, but I hope to have pointed out a potential pitfall for future people trying to wrap their heads around pagmo.