Merged and pushed. I opened a follow-up issue649.

No, please merge. (I didn't look at the code again; I don't think it's necessary.)

Can you open a follow-up issue to get rid of the uses of g_rng and --random-seed
and use the new RNG options instead? (We don't have to do that soon, but it
would be good to keep track of the fact that we want to do that eventually.)

I've implemented the suggested changes and fixed the compilation of rng_options. 

Any objections to merging?

Looks great! I'm done commenting on the pull request.

I changed everything as you suggested. The diff now is restricted to globals.*
and the new files rng-options.*:
https://bitbucket.org/SilvanS/fd-dev/pull-requests/12/issue648/diff#chg-src/search/globals.cc
https://bitbucket.org/SilvanS/fd-dev/pull-requests/12/issue648/diff#chg-src/search/utils/rng_options.cc

Could you maybe have look, Malte, if this is what you had in ? It should take
less than 5 minutes.

I re-ran experiments, and the result is the same (now, for sat, we gain some of
the borderline tasks):
http://ai.cs.unibas.ch/_tmp_files/sieverss/issue648-v2-opt-issue648-base-issue648-v2-compare.html
http://ai.cs.unibas.ch/_tmp_files/sieverss/issue648-v2-sat-issue648-base-issue648-v2-compare.html

The same argument holds: because it doesn't need access to the private
interface, it should be a function, not a method.

Yes, I agree the option code doesn't belong to globals. A separate file seems
best. Jendrik suggested putting it into "options/rng_option.h", but I don't like
this, because I'd like "options" to become a fully self-contained module that
can be used in other projects, too (which might not need RNGs). I would choose
"utils/rng_options.h" (plural), which would mean being in the same directory as
the RNG class, but not in the same file.

Okay. But the two helper functions "parse_rng_from_options" (returns g_rng() if
seed = -1), and "add_rng_options_to_parser" would live in rng.h/cc? Or would
these actually be (static) methods of RandomNumberGenerator? Or should they also
end up in globals (which I would find strange)?

I would use a function, not a method. Because it doesn't need access to the
private interface of the RNG class, it doesn't have to be a method.

Regarding function vs. object, if you read around a bit, the preferred way of
implementing singletons like "the global RNG" is via methods. I think the main
argument brought forward for this is that it gives some more flexibility, in
particular better control over the time of initialization. For example, if no
RNG is ever used, none is ever constructed.

We seem to be moving towards this pattern in general: the most recent additions
to our globals were both accessor methods, I think: g_initial_state() and
g_root_task().

In symmetry to these, I'd use a function called g_rng(). In the longer, I think
we want to get rid of it entirely, encapsulating the object in the method that
parses from the command line. Using g_rng() where we currently use g_rng is only
an intermediate solution, and therefore to avoid code churn I'd be happiest if
g_rng() would simply live in globals.cc like g_rng currently does: that way, we
have minimal code churn for now.

Malte, Jendrik and I are unsure about whether to use a static function that
returns a shared_ptr on a static object that lives in the cc-file, versus having
a global object (à la "extern RandomNumberGenerator rng" in the h-file) that is
defined in the cc-file. From our meeting, I remember that you said static and
shared_ptr, and we use this design currently for our root abstract task: "extern
const std::shared_ptr<AbstractTask> g_root_task();". For the timer, on the other
hand, we have "extern Timer g_timer;" declared in timer.h.

For rng, I implemented static functions that return shared_ptr to static objects
in the cc-file. Is that what you wanted? What's the advantage of using a static
method over a global object?

Updated results (with attributes out_of_memory, out_of_time):

http://ai.cs.unibas.ch/_tmp_files/sieverss/issue648-v1-opt-reparse-issue648-base-issue648-v1-compare.html
http://ai.cs.unibas.ch/_tmp_files/sieverss/issue648-v1-sat-reparse-issue648-base-issue648-v1-compare.html

For optimal, the coverage we lose is due to borderline tasks where we now run
out of memory compared to the baseline.
For satisficing, there are several domains with large runtime fluctuations,
leading to tasks being solved with the baseline and not with v1, or vice versa. 

As qualitative results did not change, I think the results are ok.

Do we need to discuss more about the interface (i.e. about the code)?

The idea that I discussed with Malte is to only use rng via the new static methods. 

Maybe we could actually think of making the constructors private. But then we
would need to add a method to create a rng object seeded with time and pid.

We actually have to use shared_ptr for the method parse_rng_from_options,
because we want to create a new rng everything we use this method, manually
specifying a seed different than -1.

I had a look at the code and noticed the following things:

- I think it's desirable not to couple the options code to the rng code. If we 
put add_rng_options() and parse_rng_from_options() into a separate module, we 
can use the rng module in other projects that don't use the options code. One 
idea would be to add a module options/rng_helper.h or options/rng_option.h.

- Do we really have to use a shared_ptr? I'd propose to construct a static RNG 
object and return a reference to it when the accessor method is called.

Yes, sorry, I realized this after going to bed yesterday but was too lazy to fix it.

https://bitbucket.org/SilvanS/fd-dev/pull-requests/12/issue648/diff

Here are the results for sat:

http://ai.cs.unibas.ch/_tmp_files/sieverss/issue648-v1-sat-test-issue648-base-issue648-v1-compare.html

We lose some coverage in some configurations, although expansions do not change.
Unfortunately, the default issue experiments do not display more information on
why an instance could not be solved. I'll try to get that information.

Hi Silvan, did you forget the link to the pull request?

I opened a pull request at bitbucket:

Can someone please have a quick look? It is a really simple patch.

Results for configurations involving randomness (so far only optimal) look good:

http://ai.cs.unibas.ch/_tmp_files/sieverss/issue648-v1-opt-test-issue648-base-issue648-v1-compare.html

To get rid of the global rng object, we want to implement a method in
RandomNumberGenerator that returns a shared_ptr to a static rng object, by
default. There will also be the possibility to pass in an Options object (which
must contain a "random_seed" option) to obtain a different copy of RNG with the
given seed. A helper method to add the "random_seed" option to a given parser
will also be provided.

The goal of this change is to allow all current users of g_rng to be able to use
their own seeded rng, if they want. The purpose of the method that simply
returns a pointer to the static rng object (without random seed option) is to
replace g_rng without the need that all users of g_rng must store their own
pointer to the new rng object as a class variable.