Merged. Thanks for the review.

Pull request and experiments look good. Once this is integrated with the new
code, I think it can be merged without further review or experiments.

I ran A* with the blind and LM-cut heuristics for the optimal benchmark
and lama, lama-first, and ehc with the FF heuristic for the satisficing
benchmarks. The coverage is unaffected in all configurations.

 
http://ai.cs.unibas.ch/_tmp_files/pommeren/issue725-v1-opt-issue725-base-issue725-v1-compare.html
 
http://ai.cs.unibas.ch/_tmp_files/pommeren/issue725-v1-sat-issue725-base-issue725-v1-compare.html

The optimal results surprised me a little. I expected runtime to go up
slightly in both cases and expected the blind search to be affected
more because LM-cut spends more time in the heuristic computation.
Instead, the times for blind search actually improved by a few percent.
Its less than the typical noise but clearly visible in the plot. The
runtime of LM-cut on the other hand increased as predicted.

  http://ai.cs.unibas.ch/_tmp_files/pommeren/issue725-blind-search_time-base-v1.png
  http://ai.cs.unibas.ch/_tmp_files/pommeren/issue725-lmcut-search_time-base-v1.png

Both effects are small enough (2-3%) that I think we can live with them
and merge this isue.

For the satisficing results, I only generated the plot for lama-first
and ehc because the timing results of lama depend on the last
successful iteration. The noise is larger in satisficing benchmarks as
usual and I don't see a large impact here.

  http://ai.cs.unibas.ch/_tmp_files/pommeren/issue725-ehc_ff-search_time-base-v1.png
 
http://ai.cs.unibas.ch/_tmp_files/pommeren/issue725-lama-first-search_time-base-v1.png

LAMA (not lama-first) is a good configuration for "iterated". For EHC, just pick
anything that we've used in experiments in the past. In older versions of lab, I
recall there were some "extra default configurations" that included some variant
of EHC, but I don't recall the details.

Thanks Jendrik. If you have any more comments, we can fix them before this issue
reaches the top of Malte's queue (I added it now).

What kind of experiments should I run? lama-first would cover a lot of changes
(LazySearch, EdgeOpenList, notify_state_transition, the global changes like
using ID in search node info, saving plans and computing their costs). That
leaves the other search methods (eager, EHC, iterated). I could do A*/LM-cut for
eager but what are good configurations for EHC and iterated?

Hi Florian, if you also want me to take a look at this, please add it to the
review queue.

I made some minor comments on Bitbucket.

I added a discussion about real_g to the agenda.

I created a pull request for the code I would like to change. The major changes are:
 * Plans (was: vector<const GlobalOperator *> now: vector<OperatorID>)
 * notify_state_transition (was: const GlobalOperator * now: OperatorID)
 * EnforcedHillClimbingSearch::reach_state and 
 * EdgeOpenListEntry (was: pair<StateID, int> now: pair<StateID, OperatorID>)
 * EnforcedHillClimbingSearch::insert_successor_into_open_list (parameter was: 
   const GlobalOperator * now: OperatorID) 
 * LazySearch::current_operator (was: const GlobalOperator * now: OperatorID)
 * SearchNodeInfo::creating_operator (was: int now: OperatorID)
 * A couple of removed unused forward declarations

Let me know if you prefer some of these changes in their own issue.

https://bitbucket.org/FlorianPommerening/downward-issues/pull-requests/35

I think both of them should disappear from SearchNodeInfo eventually -- not
least because many configurations don't need them, and they waste memory. In
configurations without a passed-in cost bound, certainly we don't need two g
values. And in non-reopening greedy search configurations without a passed-in
bound (e.g. lama-first, or more generally most lazy_greedy configurations), we
need neither.

Regarding the way ahead with this issue, I suggest you repurpose it for whatever
you want to integrate right now. No need to open a separate issue.

In SearchNodeInfo, we store both values, so we currently need access to both. If
one of them would disappear into the evaluator, that would help. The change in
perspective sounds good for that.

But it also sounds like a larger operation. I mainly started this issue to get
some quick fixes and cleanup commits in that are possible now that we have
operator IDs. The other changes I made so far still look good to me. Should I
open a new issue for them, so we can merge them?
Either way, I suggest we put this one on ice until we figure out how to handle
real_g. Jendrik, can you put this on the agenda for the next meeting, please?

Are you working from the assumption that the "normal" task that the search
algorithm operates on is the one that provides "g", and we need some special
mechanism to get at "real_g" because it might differ from "g"?

If yes, it might be possible to shift perspective and make the task that
provides "real_g" the "normal" one from the perspective of the search algorithm,
and "g" is based on a GEvaluator operating on a transformed task. (I think
GEvaluators currently cannot work on transformed tasks because task
transformations are introduced further down in the ScalarEvaluator/Heuristic
hierarchy, but this is something we could change.)

Would this help?

The point is that there are not so many things that "g" is actually used for
during search, and it might be quite possible to make "g" just another aspect of
the evaluator that is used within the open list.

Ahh, this might not so easy as long as SearchNodeInfo still stores real_g. We
can only get this cost from the GlobalOperator, so we cannot pass a
OperatorProxy to SearchNode::open and similar functions. We could pass in a
GlobalOperator and an OperatorID but that feels like adding one hack to get rid
of another.

Now that issue688 is merged, we can start to clean up some transitional code.
In this issue, I would like to get rid of get_op_index_hacked which looks up the
ID of a global operator. It is currently used by all three searches in
situations where the ID should be available.

In this context, I would like to use OperatorID instead of int for
SearchNodeInfo::creating_operator and EdgeOpenListEntry, the two places where
the function is used.