Thanks! :-)

I prefer the first option and implemented it.

Yes, I looked at the code and understand where this is coming from, but we shouldn't expose a random seed option to the user for components that are not random.

Two not too invasive options:

- Make the RNG parameter to VariableOrderFinder optional (e.g. passing a pointer rather than reference) and assert that it is non-null for the options that use randomness.

- Rather than getting an RNG from the command line in the greedy generator pattern generator, have it create its own RNG object and pass this to the variable order finder.

The drawback of the second one is that creating an RNG object for the Mersenne Twister is actually quite expensive.

I fixed the first (added the option twice).

The second and third now have a random_seed option because the greedy pattern generator uses VariableOrderFinder to pick variables in the "causal graph level" order. With the change of the issue, VariableOrderFinder now requires a random_seed, which, of course, is only used with the random order options of VariableOrderFinder. 

Maybe a cleaner solution would have been to also expose the variable order type of the greedy pattern generator, together with the random seed.

The combo generator uses the greedy generator.

This triggered some doc changes that look suspicious.

On the MergeTree page, the "linear" plug now has two arguments called "random_seed", one at the start and one at the end of the argument list.

On the PatternGenerator page, "greedy" now has a random_seed option even though the configuration doesn't use random numbers (as far as I can tell).

On the PatternCollectionGenerator, "combo" now has a random_seed option even though the configuration doesn't use random numbers (as far as I can tell).

Merged.

Sorry, I indeed misses that the notification email was longer than I saw. I addressed the comments and added a change log. I will merge later if I don't hear anything else.

I think you missed two of my comments (regarding emplace_back() and the "algorithm" include). Apart from these, looks good to merge.

I've had a quick look, no objections to merging once you add a change log entry (in this case a "for developers") one. Not sure if everybody saw it, we recently discussed on Discord to have a changelog entry for every issue, whereas previously we left it a little bit open for internal changes like these. (Of course, feel free to reopen this discussion.)

I addressed it.

Thanks for looking into this! I left some comments on GitHub.

Pull request: https://github.com/aibasel/downward/pull/62

VariableOrderFinder currently uses std::random_shuffle instead of our own RNG class. This should be changed.