OK, thanks! I ran another experiment for the latest revision:

https://ai.dmi.unibas.ch/_tmp_files/seipp/issue786-v3-issue786-v3-base-issue786-v3-compare.html

Since there were no unexpected regressions, I went ahead and merged this.

Yes, I'm fine with the changes.

Thanks for the review, Silvan. I'm done addressing your comments. Do you think 
this can be merged?

I'm done commenting at bitbucket.

It looks OK to me, but I'm not confident, as I don't know the sampling code that
well (or not that well any more). To review this properly, I would need a lot
more time, and I don't want to keep this up.

If it's OK with you, I'd like to delegate the approval to another reviewer of
your choice. I think Silvan could be a good reviewer for this code, but if he is
not available, feel free to ask someone else. If nobody else is available, feel
free to put this into the review queue again and I'll try to find the necessary
time ASAP.

I merged the default into the issue branch.

Will do.

I am looking at this pull request now. There is a merge conflict that makes
reviewing difficult. Can you have a look at that?

Here are the results. I don't see any big changes:
http://ai.cs.unibas.ch/_tmp_files/seipp/issue786-v2-issue786-v2-base-issue786-v2-compare.html

Thanks for the comment, this idea works great! I already implemented it in 
issue780 and checked that the new code still computes the same saturated cost 
functions. Once I have removed the old code, I'll revive issue780 and will ask 
for a light review.

The issue at hand (issue786), however, could be reviewed now. I have reverted the 
changes to pattern_database.{h,cc} and used the new RandomWalkSampler class for 
iPDB and the potential heuristics. Experiments are running.

There are multiple ways to approach this. I would probably use a different
technique for computing the transitions of a projection. There is no need to
test preconditions for a given state; it's more efficient (and not complicated)
to go operator by operator (rather than state by state) and generate all the
states that match the preconditions of the given operator. This can also be
easily done directly in the space of perfect hash values, i.e., without the
vector-based state representation.

I agree. In the SCP code, we currently use AbstractOperator::concrete_operator_id to compute the saturated cost function 
(https://bitbucket.org/jendrikseipp/downward/pull-requests/88/issue780/diff#Lsrc/search/cost_saturation/projection.ccT312). 
The function uses the MatchTree class. Do you see a way of implementing the function efficiently without 
AbstractOperator::concrete_operator_id and without duplicating the MatchTree code for a new kind of operator class? Should I 
create a new operator class and write a new MatchTree class for it (in issue780)?

This is a step in the right direction, but I think you're still trying to make
the AbstractOperator class do something it was not designed for. 

It used to have one purpose, and now it has at least two and perhaps three, and
these purposes don't work well together because they really require different
kinds of data to do their thing.

One example of this is that for some time in the lifetime of the abstract
operator the regression preconditions make sense and at other times they do not.
The existence of the concrete_operator_id attribute that sometimes makes sense
and sometimes doesn't (is invalid) is another symptom of the same thing. The
class can be in a number of different states now, and this greatly reduces its
cohesion.

The memory savings you see are a symptom of keeping the AbstractOperator for
something it is not intended for. It is there as a representation of the
operator semantics that is needed only for constructing the PDB and should be
discarded immediately afterwards. An abstract operator that does not know its
preconditions does not make sense for this purpose; by the time we don't need to
know this information any more, the whole class has outlived its usefulness and
all AbstractOperator instances should be discarded. So the memory savings you
see are a symptom of keeping the objects around for too long, perhaps because
you now want to use them for another purpose they were not originally meant for.

(Perhaps the name "AbstractOperator" is misleading because it sounds a bit
misleading regarding the role of the class.)

I don't want to drag you into an online discussion, but maybe I can make a suggestion on how to improve the patch. If you like it, I can implement it. Otherwise, we 
can continue the discussion offline.

1) I extend the documentation for AbstractOperator::concrete_operator_id.
2) I remove AbstractOperator::~AbstractOperator() to obtain the move constructor automatically.
3) I don't release the memory of AbstractOperator::regression_preconditions.
4) I remove the public "std::vector<State> sample_states_with_random_walks()" function and use RandomWalkSampler in the iPDB and potential heuristics code instead.

Regarding the regression preconditions:

I compared keeping the vector (v1), swapping out its contents (v2, not shown in report) and storing it in a unique_ptr and releasing the pointer (v3) here: 
http://ai.cs.unibas.ch/_tmp_files/seipp/issue780-regression-preconditions-issue780-v1-issue780-v3-compare.html . While the peak memory usage goes down by hundreds 
of MBs for example in Scanalyzer and Tetris, the relative memory savings are less pronounced and I think we should prefer code quality over these memory savings.

I left some general comments on bitbucket, but this needs further discussion. If
we cannot find time for this within the next week, please add to the review queue.

I ran an experiment evaluating the effects of the change on cegar() and ipdb(): 
http://ai.cs.unibas.ch/_tmp_files/seipp/issue786-v1-issue786-base-issue786-v1-compare.html

Memory and time usages of ipdb() are more or less unaffected. cegar() needs a bit more space 
since the RefinementHierarchies now store abstract state IDs. The total_time also increases 
slightly, probably because the abstract state IDs have to be set. Most of the new code will 
only be executed when we merge issue780.

I made a pull request at https://bitbucket.org/jendrikseipp/downward/pull-requests/89 .

We decided split issue780 into two parts:

1) one that contains the changes to the existing code that is necessary to
prepare the new code (this issue)
2) one that contains only the new code (issue780)