Merged. Thanks, everyone! :-)

So the code is now in place. The next step will be to use the auto-generated
docs. I'll write to the downward-dev list about this.

> Can you initiate a code review on Rietveld? I've started looking at the code
> and found a few things.

Hold the horses, I was wrong -- the parts that looked strange was unrelated to
this issue. I'll look at the remaining source files and write again about
whether or not a a review ticket is necessary.

I've pushed some changes to the repository (merged from master repository).
Can you initiate a code review on Rietveld? I've started looking at the code and
found a few things.

I ran some tests on an unrelated issue in lab and used this experiment as a
test. The results are for the current head and the merge of this revision and
issue232, i.e. both revisions include issue344 now.

http://ai.cs.unibas.ch/_tmp_files/pommeren/report-compare-scores-ff-344.html

In this case the runs were not randomized.
Both revisions solve all tasks now and report the same number of expansions (in
the last iteration?) but have different costs and search times.

Can you send me the experiment file, too, if it is not in the repository? (For
the exact option strings.)

https://bitbucket.org/flogo/downward-issues-issue232

I just gave you write access. If anyone else wants access, tell me.

That's better than the other way around. :-) Still, worth looking at the code
since it shouldn't affect performance. Maybe we're just (un)lucky. Where can I
find the code?

"better" means the new version (6d3d65149ae6) is better than the default branch
(992bfcdfab6d).

Hmmm, 46 out of 77 (ignoring the "same coverage" results) would be significant
at around p = 0.055 for a one-tailed test and p = 0.110 for a two-tailed test.
Still in the "maybe a coincidence, maybe not" range, I'm afraid.

To be clear: does "better" mean that the new version is better or that the old
version is better?

http://ai.cs.unibas.ch/_tmp_files/pommeren/report-compare-scores-ff-randomized-100.html

Better coverage: 46/100
Worse coverage:  31/100
Same coverage:   23/100

Let's go with option 2, then.

I'm not really familiar with this patch (I just did the experiments) so I'm not
sure if I can spot anything there. I could do option 2), though. It should not
take too long if we continue to look at the parking domain only.

Hmmm... if we only look at coverage and compare the cases where coverage
increases to the cases where coverage decreases, we'd expect this to be 50:50.
However, we get 7 out of 8 increases, which for a one-tailed test is significant
at around p = 0.035, i.e. within the magic 5% (or p = 0.070 for a two-tailed
test, slightly outside the 5%).

This can still be coincidence, but it probably pays to be careful. The runs were
shuffled randomly, right?

I see two options here:
1) Have a good look at the code to see if something could explain the change.
2) Generate more data, say with 100 runs, to increase the usefulness of the
statistics, then do 1) if the results still look non-random.

Florian, which do you prefer? I would prefer 1) since it looks like less work
for us.

Ok, I put the results in the same report as before:

http://ai.cs.unibas.ch/_tmp_files/pommeren/report-compare-scores-ff-randomized.html

Thanks! I'm not sure this allows us to say if the difference in time is noise or
not, though. I think we really ought to repeat the experiment multiple times.

I think the easiest way to do this in lab is to define multiple configs with
names like "iterated-wa-ff-1", "iterated-wa-ff-2" etc. that use the same
parameter strings.

Using 10 copies of each config, together with random permutation of the jobs,
would ensure that each runs has a good chance of landing on a fully loaded
machine, which should further reduce noise.

The updated results for the parking domain with randomized run ordering are in
this report:

http://ai.cs.unibas.ch/_tmp_files/pommeren/report-compare-scores-ff-randomized.html

The cost still increases, but the search times vary between better and worse
values. The difference in search time for the same amount of expansions is
sometimes quite drastic.

Fluctuations in runtime of this magnitude are conceivably due to "grid noise",
i.e., might be explained by what's happening in the other cores of the grid at
the same time. One thing we can do is repeat the experiment a few times (say 3-5
times), ideally at different times.

It would also be very useful to randomly permute the run files so that the noise
becomes more random. If we run configuration-by-configuration, noise can be
highly systematic, and systematic noise is much worse than random noise.

Would it be much work to add a random job permutation facility to lab? This
would be useful for all kinds of experiments and might even be good as a default
setting.

It would be nice to keep the numbering of run directories as it currently is,
because I often rely on the fact that instance x+1 of a domain comes after
instance x in the run dirs etc., so maybe this would best implemented by
modifying the master run file so that it performs a random permutation from grid
task ids to runs.

I'm attaching a script that shows how something like this can be done in bash.

I merged in the current default branch again and repeated the experiments for
iterated search that show up as unexplained errors in the last report:

http://ai.cs.unibas.ch/_tmp_files/pommeren/report-compare-scores-ff.html

Coverage stays the same here, but costs are lower in one scanalyzer task and
higher in six parking tasks. The log files show that the reason are different
expansion rates just as we have seen in the other report. In the parking tasks
for example, the new version expands states up to 30% slower. Strangely enough
the old version expands states sometimes faster and sometimes slower.

I tried reproducing the differences locally but was not successful so far.

Note: now that issue123 is almost merged, any references to "Synergy" should be
removed. It seems that there are some manual references of this kind in a few
places (at least that's what it looks like from the generated documentation that
is currently in the wiki). "grep -iR synerg" should probably find all these.

Uploaded the evaluation files to

http://ai.cs.unibas.ch/_tmp_files/pommeren/issue232-opt-report-compare-scores.html
(46MB)

and

http://ai.cs.unibas.ch/_tmp_files/pommeren/issue232-sat-report-compare-scores.html
(95MB)

(WARNING: LARGE HTML FILES)

I did not have a detailed look yet, but the results for the optimal configs look
good to me. In the satisficing track there seems to be an unsupported config
which is probably just a wrong config definition in the experiment. But there
were also some unexplained errors. @Jendrik: we should look into this, but since
this has nothing to do with the issue, lets discuss it offline.

I started 2 large experiments to evaluate this. I had to merge in the current
head of the default branch to use the new exit codes (issue338) this also
included the clarification Jendrik mentioned. For now I only pushed this merge
to my bitbucket account so I can use it for the experiment. Should I push it
somewhere else?

The experiment will now compare revision d572d66eb898 to a new revision
(b85e39ef3bbb), which is merged from d572d66eb898 and 9972c711f3c8. I'll report
the results when they are done (probably monday).

We should also clarify here that the g-bound is exclusive (issue379).

Changes due to code review are pushed. The next step will be the evaluation,
which should compare the head of the issue232 branch to its youngest ancestor on
default (473abcdaa340).

Thanks for the review, Erez! I'll add a few comments on the tracker and
incorporate the changes directly.

Patch looks good to me. I've also merged in the changes from default and pushed
the branch to the master repository (but not yet merged back to default).

The next two steps are an independent look at the code (because the patch is
large and global) and an experiment that exercises the new code. I've asked for
volunteers on downward-dev for these two things.

Code is up for review at http://codereview.appspot.com/5311065/ .

Important: since default values are now strings, it was necessary to change the
add_option interface slightly because of ambiguity issues. In general, the
default value string is now always given after the help string (but help is
allowed to be empty).

>   1) getting all the pages we want to change from the server and then
>  2) sending the ones that have changed.
> One drawback of this is that the "getting" part might fail if some of the pages
> we want to update don't exist yet.

Good idea, this seems to work fine! The getting part is no problem, because the
script requests a list of all pagenames from the wiki anyway.

> auth_methods_trusted = ['xmlrpc_applytoken']

I'm a bit reluctant to do that since it would apply to all users, not just our
bot, so the people who try to hit the wiki with spambots would no longer be shut
out by the surge protection mechanism. (And unfortunately wiki spam is a real
problem -- they even regularly create accounts on our wiki.) They cannot write
to the wiki of course, but we have had cases where excessive requests by bots
brought the server to its knees.

I looked through the MoinMoin code a bit and found out a few more things about this:

1) Response code 503 indeed seems to be generated only by SurgeProtection.

2) The HTTP response when surge protection is triggered includes a special
header ("Retry-After" or something similar) that indicates how long the user
should wait until trying again. So we could use this information in our code
rather than waiting for a prespecified time. Unfortunately it doesn't look easy
to get at the headers, but it should not be impossible either, e.g. by using
xmlrpclib's feature of using a custom Transport class (see
http://docs.python.org/library/xmlrpclib.html).

3) An alternative idea is to avoid doing too many XML-RPC requests in the first
place. I think the best solution would be to transmit all pages in a single
xmlrpc call. This should also require fewer HTTP roundtrips and should hence be
much faster. The reason why we're not currently doing this is that this doesn't
work if some of the pages are unchanged. But we could work around this by first
  1) getting all the pages we want to change from the server and then
  2) sending the ones that have changed.
One drawback of this is that the "getting" part might fail if some of the pages
we want to update don't exist yet. If that also prevents the other gets from
succeeding, we'd need some kind of fail-safe for that case that pushes the pages
individually then or whatever. But that should be a rather rare case. And maybe
a get of a non-existing page doesn't actually fail that noisily -- might be
worth trying out.

But I'm also happy with less complex solutions, including the
auth_methods_trusted setting or increasing the general surge limits somewhat.
Let me know what you prefer.

P.S.: Found out that one can not put users in the trusted-group manually.
Instead, one has to define auth_methods_trusted in the config, e.g.
auth_methods_trusted = ['xmlrpc_applytoken']

Could you put the XmlRpcBot into the "Trusted"-group? 
http://hg.moinmo.in/moin/1.8/raw-file/1.8.7/docs/CHANGES
According to this changelog (search for "trusted authentication"), trusted users
can't trigger surge protection, at least that was the case in version 1.70.

> A solution may be to retry after 503s, with progressively longer sleeps in
> between. Simply sleeping 10 seconds after every get/send also seems to work.

I'd go with the first solution; if it turns out to be problematic, I can crank
up the surge protection request limits and see if that makes a difference.
Unfortunately it's apparently not possible to set different limits per user, or
else we could just set a very high limit for the XmlRpcBot user.

For what it's worth, the MoinMoin documentation mentions that surge protection
(see http://moinmo.in/HelpOnConfiguration/SurgeProtection) will cause 503
responses when there are too many requests in a short time (see
http://moinmo.in/HelpOnConfiguration/SurgeProtection). It also mentions that
there will be an accompanying explanation, but maybe this is not the case for
XMLRPC requests, or they are swallowed by xmlrpclib.

> For example, is it possible to access the complete HTTP response somehow, e.g.
via some attribute of the exception object

This is everything I could get:

URL: www.fast-downward.org?action=xmlrpc2
HTTP/HTTPS headers: Date: Tue, 18 Oct 2011 14:28:30 GMT
Server: Apache/2.2.16 (Ubuntu)
Retry-After: 3600
Content-Length: 410
Vary: Accept-Encoding
Connection: close
Content-Type: text/html

Error code: 503
Error message: SERVICE UNAVAILABLE

(Same message for both get- and send-requests)


Also, we're not the only ones with this problem:
http://www.mail-archive.com/moin-user@lists.sourceforge.net/msg02969.html
(no real solution - they reverted back to moinmoin 1.8)

A solution may be to retry after 503s, with progressively longer sleeps in
between. Simply sleeping 10 seconds after every get/send also seems to work.

> 1. (optional) changes to formatting, if you have any comments now (otherwise
> we could deal with formatting in a new issue when necessary).

I didn't check all pages yet, but from what I've seen I don't think there's
anything that needs to be changed right now. I'll check everything again when we
land this.

> 2. Resolving the 503-Error - if you don't have any idea for a quick fix on the
> server side, I'll just try sleep()-ing long enough between requests.

This sounds like MoinMoin's surge protection feature, but it'd be good to be
sure. If there is any additional information we can get apart from this:
    "<ProtocolError for
      www.fast-downward.org?action=xmlrpc2: 503 SERVICE UNAVAILABLE>"
? For example, is it possible to access the complete HTTP response somehow, e.g.
via some attribute of the exception object?

> 3. I'll finish the code for review.

OK.

1. (optional) changes to formatting, if you have any comments now (otherwise we
could deal with formatting in a new issue when necessary).
2. Resolving the 503-Error - if you don't have any idea for a quick fix on the
server side, I'll just try sleep()-ing long enough between requests.
3. I'll finish the code for review.

What is the next step to move this forward?

New version at http://www.fast-downward.org/DOC/Overview .

> 1) The meaning of the different properties should be explained somewhere near
> the top, as on http://www.fast-downward.org/HeuristicSpecification ...

The script now keeps everything before the TableOfContents, so this part
can be edited in the wiki.

> 2) Some things should be links but aren't, e.g. [LamaFFSynergy] and
> [LPBuildInstructions] in the section on the landmark count heuristic. That's
> probably because it's not recognized as WikiSyntax due to several consecutive
> upper-case letters?

Should be fixed (but have not checked all pages). The script pulls a
list of page titles from the wiki and inserts links based on this.

> 3) I'd like to preserve some more of the typesetting we have on the current
> pages, e.g. using a typewriter/fixed-width font for "admissible=true" in "only
> used when admissible=true", and using block-like rather than in-line {{{...}}}
> structures for the shrink strategy examples in M&S.

I uploaded a suggestion for the shrink strategy examples - see
DOC/Heuristic.

4 and 5 should be fixed.

I sometimes get an 503 error in the middle of running the script:
xmlrpclib.ProtocolError: <ProtocolError for
www.fast-downward.org?action=xmlrpc2: 503 SERVICE UNAVAILABLE>

Maybe there are too many queries at once.

I checked the heuristic specification page only so far. Looks excellent! With a
few tweaks, I think we can use this to replace the current documentation soon:

1) The meaning of the different properties should be explained somewhere near
the top, as on http://www.fast-downward.org/HeuristicSpecification (the text is
below the table of contents there, but could be moved elsewhere). Not all of
this is standard terminology ("safe" is terminology I made up), so an
explanation would be good.

2) Some things should be links but aren't, e.g. [LamaFFSynergy] and
[LPBuildInstructions] in the section on the landmark count heuristic. That's
probably because it's not recognized as WikiSyntax due to several consecutive
upper-case letters?

3) I'd like to preserve some more of the typesetting we have on the current
pages, e.g. using a typewriter/fixed-width font for "admissible=true" in "only
used when admissible=true", and using block-like rather than in-line {{{...}}}
structures for the shrink strategy examples in M&S.

4) Since we're mimicking Python syntax in our notation, there should be no
spaces in keyword assignments like "admissible=false". (We'd do the same thing
in the C++ code if uncrustify supported it.)

5) It'd be nice if things could be marked as undocumented in the source files
and then not appear on the web page. The "IPC Max" heuristic is a hack to work
around issue198 that we probably don't want to expose on the wiki since it's
only a temporary measure. (Well, hopefully.)

6) I think we should use a separate namespace in the wiki for the documentation
-- maybe something like .../Doc/... -- and come up with a more consistent naming
scheme. for example Doc/LandmarkFactory for landmark factories, Doc/Heuristic
for heuristics, etc.

7) I also haven't looked at the code yet, only the resulting web pages.

New version, split into several pages:
http://www.fast-downward.org/AUTODOCOverview

Edit logs are not supported by MoinMoin's xmlrpc interface. There's a
plugin for this, but I couldn't get it to work on my local wiki.

[Landmark graphs]

Right, having options listed that are not relevant is ugly. Maybe open an issue
to address this later? We'll probably mess with the landmark code more soon, so
I don't think it's advisable to change the documentation reporting right now.

[XMLRPC]

Enabled. I'm attaching an example script. Two notes:

1) This needs the password for the XmlRpcBot account in a file called
"downward-xmlrpc.secret" alongside the code. I'll send you the password file in
separate email, as it should not be published in the tracker or in the
repository (so don't commit it). We'll have to think about how to deal with the
password later.

2) The script has the functionality to upload multiple pages at a time with a
single "send_pages" call, but that's not actually useful since we get an error
when a page is uploaded that has not changed since last time, and if that
occurs, the pages that are sent later on in the sequences are not processed. 

One could work around that by making the page content unique (e.g. by adding the
hg revision to the page somewhere), but I think it's best to avoid such spurious
updates to make the wiki history cleaner and easier to follow. So it's probably
best to send one page at a time.

I didn't try to find out if there's a way to set the wiki edit log entry. If
it's possible, it would be nice if it were set to something meaningful.so, this
would be nice.

> - Regarding the landmark graphs, I don't actually mind repeating the common
> options (is that the bit you consider ugly, or is it something else?) 

This, combined with the "relevant options" - part. Wouldn't it be nicer
to just have the relevant options for each landmark graph?

> - Should I enable that [xmlrpc]?

Yes, please. 

> I assume that at the moment you cut-and-paste the text by hand?

I use a local wiki for testing, but the version on the online wiki was
pasted manually, yes. In the latest version, there's one page for each
category, so cut-and-paste would be tedious.

Looks very good! I didn't check it thoroughly yet to see if there's anything
missing that I would consider important, but this is definitely the way to go.

Before we commit to this, can the others have a look at the page that Moritz
linked and share their thoughts?

Some comments:

- Should be "landmark graphs", not "landmarks graphs" -- we're slowly getting
rid of all these extra "s"s now.

- Regarding the landmark graphs, I don't actually mind repeating the common
options (is that the bit you consider ugly, or is it something else?) since it's
probably more user-friendly than our current way, where you have to look in
several places to find all relevant options. It might be nice to somehow
visually say that these are "inherited" options, but I wouldn't lose much sleep
over that at the moment, especially since we need to refactor some related
aspects of the LM code anyway.

- I used XMLRPC edits with MoinMoin before and it's definitely the nicer of the
two options since there's much less need to mess with the server. It's also
probably simpler. Should I enable that? I assume that at the moment you
cut-and-paste the text by hand?

Encoded more information, see
http://www.fast-downward.org/AutomaticDocGenTestPage .

Missing:
* equivalent specifications for the search engines - this could be moved
to a separate page
* separate pages - only missing because it's quicker to test the output
this way
* wiki-internal hyperlinks - can be done by a search-and-replace.

Slightly ugly:
* the common options for the landmark graphs

I switched to txt2tags-markup for the output. We already use this for
the reports. txt2tags can be converted to moin, and several other
formats.

Updating the documentation could be reduced to calling a python script.
But the script needs to reside on the wiki's host, or else be allowed
access via xmlrpc (see http://moinmo.in/MoinAPI/Beispiele#xmlrpc ).

Sounds good to me.

> Which functionalities would we need to add to the code to support
> something like
> this? 

Currently, the following information is missing:
-real name (e.g. "Selective-max heuristic" instead of "selmax")
-notes/synopsis
-supported language features
-properties

None of these seems to be obtainable directly from the code. I think one
would need to supply this information manually in some way -- a possible
way is to extend the parser interface and supply the information from
within the _parse-methods.

That's a very interesting start. I think a good milestone would be to enable
generating http://www.fast-downward.org/HeuristicSpecification (or something
with equivalent or better content and layout) automatically. I think that the
"common options" don't need to be set apart as they currently are; that was
mostly done to make editing the page more convenient.

Which functionalities would we need to add to the code to support something like
this?

Implemented a basic version of this today, result so far:
http://www.fast-downward.org/AutomaticDocGenTestPage

The only real problem is how to store the currently missing information,
e.g. when is heuristic x admissible etc. 

The rest should be just a minor refactoring of the doc generation code,
to allow different formatting strategies.

Setting the same nosy list as for issue104, as this one was spawned off that
one. (But feel free to unnosy yourself.)

$ ./downward --help

already produces most of the info of the documentation we have on the wiki. If
we could extend it to generate *all* relevant info with proper wiki syntax (or
something that could be automatically converted), then we could generate the
docs automatically.

Advantages:

 * less work to update
 * more likely to match the code
 * allows more easily creating snapshots of the documentation for
   a particular planner version (right now people who want to see docs for
   anything but the current default head have a bit of a problem).