We now have a reimplemented option parser, and from what I can tell this issue is resolved.

We discussed this today and concluded that the way to fix most of the issues below 
is to write a proper parser (along the lines of msg3149) checking for both 
syntactic and semantic (e.g., double arguments, non-existent arguments) errors in 
configurations.

The option parser crashes with the following arguments:
--heuristic "hipdb=" --search "astar(hipdb)"
I think it should show throw an error if after "=" nothing valid can be parsed
(and empty is something invalid).

Another option parser curiosity is that it allows passing parameters multiple 
times. I think this should be an error.

Moritz, could you please send us your patch? It doesn't matter at all in which 
state it is. We just want to avoid repeating the work that you have already done.

There is another example of the option parser should complain: it currently
accepts arbitrarily many commas as parameters:

astar(merge_and_shrink(,))
astar(merge_and_shrink(,,,,))
astar(merge_and_shrink(,shrink_strategy=shrink_bisimulation,,merge_strategy=merge_dfp))

all work correctly.

Hi again, Moritz! We currently have a HiWi student who could integrate your patch 
into the current Fast Downward version. Do you think you could post the patch 
somewhere?

In principle you could explain the changes to me, but I'll be away until the 
beginning of February as well, and understanding the changes might be hard 
since I'm not familiar with the current option parser code. 

Maybe you can upload the code somewhere, give us access and we'll continue the 
discussion in February?

Sounds excellent to me! With the caveat that I'll be unavailable until mid
February (but I don't have to be part of the code review).

Hi, sorry for disappearing. I started a full time job shortly after my last
message. Which has taken up all my focus (and still does, mostly).

My suggestion is as follows:
1) I upload the current version of the code (have to unstash my old computer
first). All mentioned issues should be fixed, but there are still a few things
to adapt, since the parse tree structure changed. 
2) We do a code review via TeamViewer/Skype, where I explain to Jendrik or Malte
what I changed, and what still needs to be done. 
3) Then one of you finishes it.
The review might also make it easier for you to extend, modify or replace the
parser in the future.

Ok?

You're right. I created issue507 for this.

If we want to borrow additional ideas from Python, Python has "keyword-only"
arguments, which also solve the problem of not changing meaning if additional
options are added. For the large majority of options, we by convention always
use the keyword form anyway. Depending on the type of object, we usually use the
non-keyword positional form only for 0-1 arguments. Maybe keyword-only should be
the default and possibly positional arguments should be the special case. But
either way, this is probably a separate issue, right?

Patrick just stumbled over a parser subtlety. I'll use a config that is present 
in the master branch to demonstrate:

When a user wants to run lazy_greedy with ff() and cea() he might forget the 
square brackets:

./downward-release --search "lazy_greedy(ff(), cea())"

The parser happily accepts this and cea() is used to calculate preferred 
operators. We should maybe think about ways to prevent such subtle usage 
errors.

I found another oddity concerning the parser. Currently, the parser accepts 
arguments of type double with arbitrary text appended to it, e.g. astar(lmcut(), 
max_time=1aha).

Sure, I can run some experiments.

At the moment we don't allow C++11 functions in the code. I would suggest adding 
comments where their usage would benefit the code (maybe only a single 
explanatory comment).

Used the suggested changes, with some modification to the numbers (allow initial
signs, and a leading dot).

Nearly done. Looks like I can get rid of the external dependency tree.hh.

Jendrik, could you run a few performance regression tests for me if I push the
code to some repository?

Btw, what is the current policy on the 'move' keyword? There are a few places in
parse tree transformation where this would be useful (but of course not necessary).

As in most programming language grammars, I would suggest to separate the
"AlphaNumeric" token into two kinds of tokens: pure numbers vs. identifiers. So
for example "3=foo()" should not be a valid "KeyValue" because 3 is not a valid
identifier, and similarly 22(foo) would not be a call because 22 is not an
identifier.

Identifier would be the regex [a-zA-Z_][a-zA-Z0-9_]* (as e.g. in C).

Number would be [0-9][0-9]* (for integers) or [0-9][0-9]*.[0-9][0-9]* (for
floating-point numbers).

I think the grammar would use Identifier in all places where it currently uses
AlphaNumeric, except in RHSParam, where both Identified and Number would be allowed.

Looks good to me. Maybe we should come up with a better name for "Call", but I 
can't think of one now.

Correction, should be: Call -> AlphaNumeric(ParamListHead)

Found some time, nearly complete now.

Here's how I formalized FDs configuration language (ignoring predefinitons):
Call -> AlphaNumeric(ParamList)
AlphaNumeric -> any alphanumeric sequence (including '_')
ParamListHead -> ParamList | ""
ParamList -> Param, ParamList | Param
Param -> KeyValue | RHSParam
KeyValue -> AlphaNumeric=RHSParam
RHSParam -> Call | List | AlphaNumeric   //parameters that are assignable to a key
List -> [ParamListHead]

Jendrik, could you take a quick look at these rules and let me know if you see
any flaws, or if something is unclear?

Hello Jendrik,

I'm still reading the mails.

The current parser is a refactoring of the original parser, sharing the ad-hoc
core parsing mechanism (which transforms the config string into a tree). While I
can fix the issues you mentioned by hacky workarounds, this mechanism should
better be replaced by something like a top-down parser. I wanted to to do this
after issue232 was merged, to avoid conflicts. Was traveling for the last few
months though.

Anyway, I can take a look at the code soon, within 4 weeks max.

Another parser issue: The following config produces a segfault:

--landmarks 
"lmg=lm_hm(reasonable_orders=false,only_causal_landmarks=false,disjunctive_landmarks=true,conjunctive_landmarks=false,no_orders=
true,m=1,cost_type=0)" --heuristic "hLM=lmcount(lmg,admissible=true,pref=false,cost_type=0)"  --search 
"lazy(single(sum(g(),weight(hLM, 3))),preferred=[],reopen_closed=false,cost_type=0)"

I would like to revive efforts for fixing this issue. Moritz, are you still 
reading the tracker emails? Could you point us in the right direction here? What 
exactly do you mean in msg2336: do you already have fixes for some of the issues? 
What would need to be changed to "fix everything"?

If it helps, I can look into issue232 this weekend.

There are two possibilities how to go on with this issue:

1: I can fix (or already fixed) most of what Jendrik said with minor changes to
the code.

2: to fix everything, I'd need to change a bigger part of the code. This is
because the parser, at the lowest level, copies the functionality of the
original parser (which accepted the same input). See also msg2007. I can do this
too, but we'd have to decide if we want to integrate issue232 first.

Hi,
since I'm currently writing my diploma thesis and my contract for 
FD-work expired, I don't actively work on FD anymore. But I've fixed 
most of the stuff Jendrik mentioned already anyway, so I plan to take 
care of this issue, sometime in the next weeks.

On 06/12/2012 03:51 PM, Malte Helmert wrote:
> Malte Helmert<malte.helmert@unibas.ch>  added the comment:
>
> I think it should be an error rather than a warning (but definitely no segfault
> ;-)).
>
> _______________________________________________________
> Fast Downward issue tracker<downward.issues@gmail.com>
> <http://issues.fast-downward.org/issue288>
> _______________________________________________________
>

I think it should be an error rather than a warning (but definitely no segfault
;-)).

Another request: 

I think it would be nice to get a warning instead of a segmentation fault when 
e.g. the parentheses don't match in the command line string:

./downward-1 --search "(astar(blind())" < output
Simplifying transitions... done!
Peak memory: 3592 KB
caught signal 11 -- exiting
Speicherzugriffsfehler (Speicherabzug geschrieben)

Sorry for the prolonged silence. Yes, we will integrate that one.
I really need an eighth day of the week.

Are there still plans to integrate issue232? I've been waiting for the code
review on that one before working on the issue here.

We did use optional, no-default parameters for M&S (lines 4.* here: http://hg.fast-downward.org/rev/d2e6a00486fd ). Must have been reverted during M&S-Refactoring.

Optional parameters without a "regular" default value are already implemented,
and this was one of the motivating examples. See issue236. I guess we're not
using it in M&S yet; patches welcome.

Something else: I think that assigning default values to int parameters when "-1" 
is specified is very error prone. I think raising an error is the better 
behaviour. If the user wants the default parameter the option should be left out 
or we should allow specifying "default".

$ ./downward --heuristic 
"hmas=merge_and_shrink(shrink_strategy=shrink_fh(max_states=-1))" --search 
"astar(hmas)" < output | grep "Abstraction size limit"
Abstraction size limit: 50000
Abstraction size limit right before merge: 50000

Probably this is restricted to shrink strategies. This behaviour has tricked at 
least Chris and me already.

Exactly.

Using the default value here will cause a Parse Error ("invalid enum argument not_greedy for option greedy at: 
shrink_strategy = shrink_bisimulation"). So I guess you mean an unconditional check for validity of the default value every time add_enum_option() is called?

Another thing: Shouldn't parser.add_enum_option() check that the default value 
is part of the enum? This is not done e.g. in shrink_bisimulation.cc line 439:

parser.add_enum_option(
        "greedy", greediness, "NOT_GREEDY",
        "use exact, somewhat greedy or greedy bisimulation");

Although I fixed what Jendrik mentioned, the parser still accepts many malformed
configs. This is because of my very ad-hoc approach for parse-tree generation.
I'll replace this with a recursive descent parser soon.

> Shouldn't Parse Errors be reported on stderr to be consistent with the general 
> behaviour?

Yes, right.

> I found the following on stdout while testing shell escaping:
> 
> Parse Error: 
> heuristic       hcea not found at: 
> preferred =     hcea
> 

The parser currently only ignores spaces and newlines. I'll change it to
ignore all whitespace.

While you're at it, you might have a look at the following:

Shouldn't Parse Errors be reported on stderr to be consistent with the general 
behaviour?

I found the following on stdout while testing shell escaping:

Parse Error: 
heuristic       hcea not found at: 
preferred =     hcea

Fixed, but I'll test some other configurations later before submitting.

Bug.

Fast Downward currently accepts "cost_type=" without a valid cost type. Is this 
a bug or a feature?

./downward-2 --heuristic "hAdd=add(cost_type=)" --search 
"lazy(alt([single(sum([g(),weight(hAdd, 7)])),single(sum([g(),weight(hAdd, 
7)]),pref_only=true)], boost=0),preferred=[hAdd],reopen_closed=true,cost_type=)" 
< ../../new-scripts/preprocessed-tasks/WORK-WORK/gripper/prob01.pddl/output
Simplifying transitions... done!
Conducting lazy best first search, (real) bound = 2147483647
Initializing additive heuristic...
Simplifying 66 unary operators... done! [66 unary operators]
Best heuristic value: 12 [g=0, 1 evaluated, 0 expanded, t=0s]
Best heuristic value: 11 [g=1, 3 evaluated, 2 expanded, t=0s]
Best heuristic value: 10 [g=2, 4 evaluated, 3 expanded, t=0s]
Best heuristic value: 9 [g=3, 5 evaluated, 4 expanded, t=0s]
Best heuristic value: 8 [g=5, 8 evaluated, 7 expanded, t=0s]
Best heuristic value: 7 [g=6, 9 evaluated, 8 expanded, t=0s]
Best heuristic value: 6 [g=7, 10 evaluated, 9 expanded, t=0s]
Best heuristic value: 5 [g=9, 13 evaluated, 12 expanded, t=0s]
Best heuristic value: 4 [g=10, 14 evaluated, 13 expanded, t=0s]
Best heuristic value: 3 [g=11, 15 evaluated, 14 expanded, t=0s]
Best heuristic value: 2 [g=13, 19 evaluated, 18 expanded, t=0s]
Best heuristic value: 1 [g=14, 20 evaluated, 19 expanded, t=0s]
Solution found!
Actual search time: 0s [t=0s]
pick ball4 rooma left (1)
move rooma roomb (1)
drop ball4 roomb left (1)
move roomb rooma (1)
pick ball3 rooma right (1)
move rooma roomb (1)
drop ball3 roomb right (1)
move roomb rooma (1)
pick ball1 rooma right (1)
move rooma roomb (1)
drop ball1 roomb right (1)
move roomb rooma (1)
pick ball2 rooma right (1)
move rooma roomb (1)
drop ball2 roomb right (1)
Plan length: 15 step(s).
Plan cost: 15
Initial state h value: 12.
Expanded 20 state(s).
Reopened 0 state(s).
Evaluated 21 state(s).
Evaluations: 21
Generated 81 state(s).
Dead ends: 0 state(s).
Search time: 0s
Total time: 0s
Peak memory: 2656 KB