Thanks for the review! This is now merged.

I did a very light review of the code and had a look at the experiments. The
code looks healthy (and better than before). The experiments show a tendency to
a slightly higher memory consumption but the overall impact is positive.

From my side, this can be merged.

(I wonder, that "error-success" does measure...)

I think it's good if we have at least a little bit of review of large amounts of
new code. Florian, you're on the nosy list: do you want to have a look?

I merged the latest master revision into the issue branch. As I said earlier, I'd 
be happy about a light review of the code, but since the diff is rather large 
and only touches the CEGAR code, we could also just rely on the experimental 
results, which look very promising (see msg7309). What do you think?

Exactly, I updated the summary accordingly.

I meant to say issue786.

If I understand correctly, this is dependent on issue637 and cannot be properly
reviewed before that is merged. Am I right? In this case, please wake this issue
up again once issue637 is merged.

I have revised the code to ensure that it produces the same Cartesian abstractions as in the default branch (without 
time or memory limits). Experimental data for this version (v8) can be seen at
http://ai.cs.unibas.ch/_tmp_files/seipp/issue637-v8-issue637-v8-base-issue637-v8-compare.html .

The time for building the abstraction(s) with at most 1M state-changing transitions is significantly reduced for many 
tasks (http://ai.cs.unibas.ch/_tmp_files/seipp/issue637-v8-init_time-cegar-original-issue637-v8-base-issue637-v8.png), 
while memory usage remains roughly the same.

I ran some tests again and the speed improvements don't come from reusing 
transition vectors, but instead from the fact that the new code stores 
AbstractState pointers in a vector instead of an unordered_set. As we've seen 
in other issues, iterating over unordered_sets is very slow compared to looping 
over vectors and we iterate over all AbstractStates in every iteration of the 
refinement loop (for resetting the g-values used for the A* search).

I evaluated two cegar() configurations, the first uses goal and landmark decompositions (cegar-lm-goals), while the 
second uses the original task (cegar-original). Both configurations use at most 1M transitions.

http://ai.cs.unibas.ch/_tmp_files/seipp/issue637-v1-issue637-v1-base-issue637-v1-compare.html

The effects of this patch are very positive, memory usage remains roughly the same, whereas total_time drops a bit for 
the cegar-lm-goals version and by a lot for cegar-original. This is probably mainly due to the new code reusing the 
transition vectors of each abstract state that is split for one of its child states.

http://ai.cs.unibas.ch/_tmp_files/seipp/issue637-v1-memory-cegar-landmarks-goals-issue637-v1-base-issue637-v1.png
http://ai.cs.unibas.ch/_tmp_files/seipp/issue637-v1-memory-cegar-original-issue637-v1-base-issue637-v1.png
http://ai.cs.unibas.ch/_tmp_files/seipp/issue637-v1-total_time-cegar-landmarks-goals-issue637-v1-base-issue637-v1.png
http://ai.cs.unibas.ch/_tmp_files/seipp/issue637-v1-total_time-cegar-original-issue637-v1-base-issue637-v1.png

I have taken a stab at this in the following pull request:

https://bitbucket.org/jendrikseipp/downward/pull-requests/92


It contains the following changes:

* Changes from issue786 which should be merged before this issue.

* The abstraction representation and abstraction refinement are now separated. 
There's a new class "CEGAR", which contains the refinement loop and refines the 
abstraction.

* AbstractStates now have an ID which allows storing information about them in 
vectors and facilitates the changes below.

* The abstract states and transitions are now separated. There's a new class 
TransitionSystem which holds the transitions and rewires them (replacing 
TransitionUpdater).

* AbstractSearch and AbstractState are now separated.

* RefinementHierarchy stores the abstract state IDs instead of h-values.


I'd be happy about a light review of the code, but since the diff is rather large 
and only touches the CEGAR code (once issue786 is merged), we could also just run 
tests.

This is a follow-up issue of issue632. It would be nice if we could decouple 
AbstractState, Abstraction and AbstractSearch a little more. Also it seems that 
AbstractState currently has too many responsibilities (storing the transitions, 
making refinements, information about domains).