I made two more small changes:

* moved maximum_heuristic.{h,cc} to "learning" since it's only needed there
  -- for a general maximum functionality, I think the more appropriate
  implementation would be as an evaluator, similar to the ones we have already.
  (I wonder why none is there actually -- there already appear to be evaluators
  for sums and weighted sums, and I'm sure Gabi implemented max functionality
  somewhere.)

* renamed g_search_space to g_learning_search_space, since it's only used for
  the learning stuff and that signals that it's specific to that and hence
  doesn't have to be universally supported. (It currently *is* only supported
  by one search algorithm...)

  I wouldn't mind making that more general at some point, but this should be
  part of a general revamp of the search space infrastructure.

I changed the implementation of the StateSpaceSample so it no longer uses a
SearchSpace. This should resolve the issues here.

Silly me - I thought resize() actually resizes the hash_map. I got rid of the
resize() for SearchSpace.

A lot of these changes are caused by the fact that I'm using a SearchSpace as a
way to store my training set. It might be better to just switch to something
else, and avoid all of these problems. I'll think about this.

> * SearchSpace::resize - actually this is used to resize the SearchSpace to
> 0, so I think resize is a better name than reserve (since it does the
> opposite)

One of us misunderstands what this does (could be me). What I think the method
does is call hash_map::resize, which is documented
(http://www.sgi.com/tech/stl/hash_map.html) as: "Increases the bucket count to
at least n." so I don't think it will actually work if you want to remove any
elements.

If you only want to clear it, maybe replace with a clear() method that calls
hash_map::clear()?

> * SearchNode::reopen - this is there for some of the probes for selective-max.
> * SearchNode::update_h - I think the contract is very straightforward - update
> the h value. I'm not sure h should only increase - this is true only for
> admissible heuristics.

I just want to be sure that the optimality doesn't break, and this is one of the
points where it can easily break without anyone noticing if one isn't careful.
Actually, it was broken in a subtle way not too long ago if I recall correctly,
causing suboptimal plans in one case of out of 100...

> However, since this goes together with the reopen
> assertions, maybe we could change the way the probes works.

I'd feel better if we could change it, but if it needs some more thought, fine
with me.

> * GeneralEagerBestFirstSearch - this is again for the probes (the
> probabilistic A* probe). In general, I think "private" members should be
> protected, because someone will want to derive your class sometime.

You could make everything public for the same reason -- someone may want to use
it at some time. :-) "protected" is part of the class's external contract, so
should be a well-designed interface, not just an implementation detail. With
access control, the rule of thumb should be to be as restrictive as possible,
and as permissive as necessary. If nothing is private in the search code, we can
change nothing without consulting how it's used in the selective hmax code,
which is too tight coupling, I think.

If it's for a single class in the learning code, declaring that class a friend
would be better. Even better would be to redesign the search space interface in
a way that the selective max code wouldn't need to rely on internals, but that
may require some more thought, so maybe better to defer it.

Discuss:
* SearchSpace::resize - actually this is used to resize the SearchSpace to 0, so
I think resize is a better name than reserve (since it does the opposite)

* SearchNode::reopen - this is there for some of the probes for selective-max.
* SearchNode::update_h - I think the contract is very straightforward - update
the h value. I'm not sure h should only increase - this is true only for
admissible heuristics. However, since this goes together with the reopen
assertions, maybe we could change the way the probes works.

* GeneralEagerBestFirstSearch - this is again for the probes (the probabilistic
A* probe). In general, I think "private" members should be protected, because
someone will want to derive your class sometime.


Todo:
* SearchSpace iterator - it should definitely be done.

Done:
* Heuristic::reset - I removed it, since it's no longer needed (it was used 
before for the landmarks heuristic, but it's no longer needed).
* lm_cut_heuristic.cc - I got rid of the commented out code.
* SearchNode::reopen - I added the assertions back. At a certain point 
of implementing EHC I had to remove them, but I think it's fine now.

There's a few changes in r3783 that I think should better be done differently:

 * Heuristic::reset:
   => not clear to me what this is for and what the contract is
   (i.e., when and why it should be called).
   Short (one-line) comment at the point of declaration would be good.
 * lm_cut_heuristic.cc:
   Generally, commented out debug code should be avoided in committed versions
   (the times stuff), and the sys/times include can be removed. (I know I put
   in some commented out debug code myself -- this should also be removed rather
   sooner than later.)

 * SearchSpace::it, SearchSpace::reset_iterator, SearchSpace::get_next_state,
   SearchSpace::has_more_states:
   I don't think SearchSpace should have a built-in iterator instance. If
   iteration is necessary, I'd much prefer to have an iterator typedef and
   begin() and end() method rather than storing an iterator within the instance.
   iterator, begin() and end() should all be const.

 * I'd prefer to call SearchSpace::resize "reserve" to make it clearer what it
   does. (It is fairly clear if you keep in mind it's a hash map, but that
   should be an implementation detail.)

 * SearchNode::reopen: Why are the assertions gone? Do these invariants not
   hold anymore? Why?

 * SearchNode::update_h: It'd be good to have some assertions and some short
   documentation of the purpose and contract there, too. For example, I assume
   that the h value may only be increased, not decreased. Right? Then there
   should be an assertion.

 * GeneralEagerBestFirstSearch: I don't like seeing all this stuff turned from
   private to protected -- protected members are almost as much of a headache
   as private ones. Who needs access to all this stuff and why?