issue232

[karpase, 2012-10-24 13:49:19]

Looks good to me.
I just had a few very minor comments.

Cheers,
Erez.

http://codereview.appspot.com/5311065/diff/56/src/search/landmarks/landmark_c...
File src/search/landmarks/landmark_count_heuristic.cc (right):

http://codereview.appspot.com/5311065/diff/56/src/search/landmarks/landmark_c...
src/search/landmarks/landmark_count_heuristic.cc:286: "supported if
`admissible=false` (but may behave stupidly and unsave)");
unsave = lead to an unsafe heuristic

http://codereview.appspot.com/5311065/diff/56/src/search/lazy_search.cc
File src/search/lazy_search.cc (left):

http://codereview.appspot.com/5311065/diff/56/src/search/lazy_search.cc#oldco...
src/search/lazy_search.cc:273: parser.add_list_option<ScalarEvaluator
*>("evals");
Shouldn't there be a second parameter saying "Scalar Evaluators"?

http://codereview.appspot.com/5311065/diff/56/src/search/learning/selective_m...
File src/search/learning/selective_max_heuristic.cc (left):

http://codereview.appspot.com/5311065/diff/56/src/search/learning/selective_m...
src/search/learning/selective_max_heuristic.cc:576:
sample_types.push_back("PDB");
Can you add descriptions for these?
Probe = stochastic hill-climbing probes of Karpas & Domshlak, IJCAI 2009
ProbAStar = Probabilistic A* sampling
PDB = Sampling method of Haslum et. al., AAAI 2007

http://codereview.appspot.com/5311065/diff/56/src/search/open_lists/alternati...
File src/search/open_lists/alternation_open_list.cc (right):

http://codereview.appspot.com/5311065/diff/56/src/search/open_lists/alternati...
src/search/open_lists/alternation_open_list.cc:23: "or at least one reliable
open list considers is a dead end. "
is -> it

http://codereview.appspot.com/5311065/diff/56/src/search/open_lists/open_list...
File src/search/open_lists/open_list_buckets.cc (right):

http://codereview.appspot.com/5311065/diff/56/src/search/open_lists/open_list...
src/search/open_lists/open_list_buckets.cc:25: parser.add_option<ScalarEvaluator
*>("eval", "scalar evaluator");
Did evals change to eval on purpose?

http://codereview.appspot.com/5311065/diff/56/src/search/pdbs/pattern_generat...
File src/search/pdbs/pattern_generation_haslum.cc (right):

http://codereview.appspot.com/5311065/diff/56/src/search/pdbs/pattern_generat...
src/search/pdbs/pattern_generation_haslum.cc:275:
parser.document_synopsis("Improvement(?) PDB", "");
This is from Haslum et. al, AAAI 2007.
I don't know what to call it either.

http://codereview.appspot.com/5311065/diff/56/src/search/search_engine.cc
File src/search/search_engine.cc (right):

http://codereview.appspot.com/5311065/diff/56/src/search/search_engine.cc#new...
src/search/search_engine.cc:76: "all actions are accounted for with their real
cost");
This is duplicated.
It might be worthwhile to put this in a central place (globals?)

[malte.helmert, 2012-10-24 16:50:11]

Everything dealt with and pushed, but not yet integrated to the default branch.
I'll wait for Florian's experiments with that. Thanks again!

http://codereview.appspot.com/5311065/diff/56/src/search/landmarks/landmark_c...
File src/search/landmarks/landmark_count_heuristic.cc (right):

http://codereview.appspot.com/5311065/diff/56/src/search/landmarks/landmark_c...
src/search/landmarks/landmark_count_heuristic.cc:286: "supported if
`admissible=false` (but may behave stupidly and unsave)");
On 2012/10/24 13:49:19, karpase wrote:
> unsave = lead to an unsafe heuristic

Changed.

http://codereview.appspot.com/5311065/diff/56/src/search/lazy_search.cc
File src/search/lazy_search.cc (left):

http://codereview.appspot.com/5311065/diff/56/src/search/lazy_search.cc#oldco...
src/search/lazy_search.cc:273: parser.add_list_option<ScalarEvaluator
*>("evals");
On 2012/10/24 13:49:19, karpase wrote:
> Shouldn't there be a second parameter saying "Scalar Evaluators"?

Yes, I think so. Added.

http://codereview.appspot.com/5311065/diff/56/src/search/learning/selective_m...
File src/search/learning/selective_max_heuristic.cc (left):

http://codereview.appspot.com/5311065/diff/56/src/search/learning/selective_m...
src/search/learning/selective_max_heuristic.cc:576:
sample_types.push_back("PDB");
On 2012/10/24 13:49:19, karpase wrote:
> Can you add descriptions for these?
> Probe = stochastic hill-climbing probes of Karpas & Domshlak, IJCAI 2009
> ProbAStar = Probabilistic A* sampling
> PDB = Sampling method of Haslum et. al., AAAI 2007

I've added these following the example of the cost_type enum in heuristic.cc.

http://codereview.appspot.com/5311065/diff/56/src/search/open_lists/alternati...
File src/search/open_lists/alternation_open_list.cc (right):

http://codereview.appspot.com/5311065/diff/56/src/search/open_lists/alternati...
src/search/open_lists/alternation_open_list.cc:23: "or at least one reliable
open list considers is a dead end. "
On 2012/10/24 13:49:19, karpase wrote:
> is -> it

Done.

http://codereview.appspot.com/5311065/diff/56/src/search/open_lists/open_list...
File src/search/open_lists/open_list_buckets.cc (right):

http://codereview.appspot.com/5311065/diff/56/src/search/open_lists/open_list...
src/search/open_lists/open_list_buckets.cc:25: parser.add_option<ScalarEvaluator
*>("eval", "scalar evaluator");
On 2012/10/24 13:49:19, karpase wrote:
> Did evals change to eval on purpose?

Definitely looks intentional. It's been years since I understood all the details
of our evaluators machinery, but I think we should see in our tests if anything
breaks (e.g. because we're passing in a list on the command line).

http://codereview.appspot.com/5311065/diff/56/src/search/pdbs/pattern_generat...
File src/search/pdbs/pattern_generation_haslum.cc (right):

http://codereview.appspot.com/5311065/diff/56/src/search/pdbs/pattern_generat...
src/search/pdbs/pattern_generation_haslum.cc:275:
parser.document_synopsis("Improvement(?) PDB", "");
On 2012/10/24 13:49:19, karpase wrote:
> This is from Haslum et. al, AAAI 2007.
> I don't know what to call it either.

Stands for "incremental PDB", I think, but it was only an internal name until we
recently mentioned it in our SoCS paper on PDB implementations. Probably more
people understand it as "iPDB" than "incremental PDB", so I've left it at "iPDB"
and added literature references to the Haslum et al. paper and the SoCS paper.

http://codereview.appspot.com/5311065/diff/56/src/search/search_engine.cc
File src/search/search_engine.cc (right):

http://codereview.appspot.com/5311065/diff/56/src/search/search_engine.cc#new...
src/search/search_engine.cc:76: "all actions are accounted for with their real
cost");
On 2012/10/24 13:49:19, karpase wrote:
> This is duplicated.
> It might be worthwhile to put this in a central place (globals?)

Good catch! I've unified it in operator_cost.cc. The two main callers of this
(heuristics and search engines) now use common code, which required rephrasing
the documentation a bit since it was different in the two cases, but I think the
new phrasing is fine.

The third piece of related code is the lm_cost_type option of landmark
factories. I've only added a TODO there since that option should be going away
after further cleanup anyway.