Ok, done and merged. Sorry for all the branch re-opening. I'm closing the issue
again.

I don't think that name needs to be changed, but of course the parameter should
only be passed in when using an anytime (= satisficing) portfolio. An portfolio
for optimal planning can never have a previous plan and hence doesn't use this
option -- that sounds fine enough for me.

How about the option "--internal-previous-portfolio-plans"? Should I rename it too?

I suggested more or less the same solution.

Ah, right, of course the name isn't so well suited in that case. For optimal
portfolios, only one plan is generated, so the search component doesn't need to
do anything special. Perhaps rename to "g_is_part_of_anytime_portfolio"?

Florian and I noted one thing today: the --internal-previous-portfolio-plans is 
only passed for satisficing portfolios and therefore g_is_part_of_portfolio is 
false for optimal portfolios. How would you like us to handle this, Malte?

Merged. I renamed the variable g_previously_generated_plans to
g_num_previously_generated_plans as Jendrik suggested.

I agree that it can be merged.

Thanks! I also had a brief look and it looks good to me, so if it works and if
Jendrik is also happy with the code, I think it's ready to merge.

I uploaded the changes to
  https://bitbucket.org/flogo/downward-issues-issue501/pull-request/1

Jendrik volunteered to do a review.

> How should we test for this?

I suggest that the callers to save_plan pass this information as a boolean argument.

Fine with me.

> [...] or if we are conducting an iterated search.

How should we test for this?

> Does this concern any configuration besides LAMA or iterated search?

LAMA is a special case of iterated search. I assume it only concerns iterated
search. (Note that there's nothing special about LAMA -- it's a bog-standard
command-line alias for an iterated search configuration, no special handling in
the driver.)

> We could either pass "--internal-plan-counter 1" to the configurations in
> question,

That is not a solution. "--internal-plan-counter" is an internal (i.e., not
intended to be user-visible) option for use of the driver only, like all options
starting with "--internal". It's intended to be used only for communication of
the driver with the search component (mainly for portfolios).


Setting g_plan_counter to different values that must be interpreted differently
depending on the kind of search looks fragile to me. I think the semantics of
the variable wouldn't be very clear. (What would the intuitive meaning of a plan
counter of 0 vs. 1 be?) I'm also not sure if the suggested solution would
interact correctly if we combine portfolios and anytime configurations.

I think a better solution is to have it always reflect the number of previously
generated plans and rename it (along with the option) to make this clear:
"g_num_previously_generated_plans" and "--internal-previously-generated-plans"
or something shorter along those lines. The planner knows if it wants to
potentially generate multiple plans (whenever iterated search is used), so it
knows how to distinguish "sas_plan" from "sas_plan.1" in that case
automatically. For portfolio configurations, we must make sure that the planner
prefers "sas_plan.1" over "sas_plan" even for non-iterated configurations. One
way to do this is to have "--internal-previously-generated-plans" additionally
set a flag that shows we are in a portfolio. To make this clearer, the option
could be renamed to "--internal-previous-portfolio-plans".

Summary of suggestion:

- g_previously_generated_plans defaults to 0
- g_is_part_of_portfolio defaults to false
- Option "--internal-previous-portfolio-plans X" sets
g_previously_generated_plans to X and g_is_part_of_portfolio to true
- When a plan is generated, we increment "g_previously_generated_plans" and
append the counter to the plan name if we are in a portfolio or if we are
conducting an iterated search. We add an assertion that in other cases, the
counter must necessarily be 0 (or 1 if we test it after incrementing)

What do you think?

I agree with Florian. We should probably set g_plan_counter to 1 for iterated 
searches.

I'm not sure how this was handled before. Currently, there is a counter
(g_plan_counter) that is initialized to 0 but that can be overwritten with the
command line argument "--internal-plan-counter". Whenever a plan is stored, the
counter is appended to the filename (except when it is 0) and then incremented.

We could either pass "--internal-plan-counter 1" to the configurations in
question, or we could change the code of iterated search to set g_plan_counter
to at least 1. The latter has the advantage that users can still use other
values for "--internal-plan-counter".

Does this concern any configuration besides LAMA or iterated search?

$ ./fast-downward.py --alias seq-sat-lama-2011
../benchmarks/blocks/probBLOCKS-8-0.pddl

finds four plans, which are written to files sas_plan, sas_plan.1, sas_plan.2
and sas_plan.3.

They should have been written to sas_plan.{1,2,3,4}: according to IPC rules, the
planner should *either* use sas_plan *or* sas_plan.XXX, not both. Presumably
something went wrong when we changed the interaction between the driver script
and iterated search/portfolios.