Part of issue567.

For the record: this has now been silently fixed in the master.

I discovered a bug in the code introduced in this issue which lead to the feature only being correctly implemented partially. Normally, I would just fix this in default, but with our new release cylce, I was wondering whether there is reason to not do so and if such as fix should be documented in the CHANGES file.

Thanks a lot!

I left some minor comments, but this looks good to be merged.

Sure, I'll have another look.

Thanks! I polished the documentation accordingly. Jendrik, can I get you to do a review? :-)

I've left some general and polishing comments. If you can get Jendrik to do a review before merging, then I'm fine with merging this. Otherwise I'm not sure when I'll be able to take a closer look.

Added link to meta issue567. 

This is "ready for being merged" and I would like to ideally merge it before we start switching to git. Any chance to get a final look at the PR? :-)

BTW., I just saw that bitbucket diffs now have a button that shows previous comments on the same file which might be very useful in cases like this where we already went through an iteration of code review.

Added. Please let me know if you are happy with my latest changes.

> Regarding the changelog, where do I find this?

We mostly discussed this live, in the second meeting on the release. There isn't
much documentation, but check this:

http://www.fast-downward.org/ForDevelopers/ChangelogFormat

> The file CHANGES.md looks like it should only be changed once the next
> release time/name is set in stone.

We want to polish it at this point, but it will be easier to start entering some
things when they're fresh in our mind. I'd suggest adding a section for 2019.06+
and adding entries there.

I made another iteration of changes.

Regarding the changelog, where do I find this? The file CHANGES.md looks like it
should only be changed once the next release time/name is set in stone.

I left some review comments. I didn't write it on bitbucket, but please also add
a changelog entry.

Reminder :-)

I'll remind you then.

For completeness, here are the latest results:
https://ai.dmi.unibas.ch/_tmp_files/sieverss/issue914-v4-issue914-base-issue914-v4-compare.html

Silvan, happy to look at this after my IJCAI deadline (= on Tuesday). Please
remind me.

I left some comments. Feel free to ignore them as I'm not familiar with the M&S
code.

Even though this is not part of the sprint stories, I would be happy to get this
off the list :-) Anyone up for another, hopefully final review?

Yes, I misunderstood a previous comment, I guess. The code now again checks if
all states are goal states and additionally checks if the factor has been pruned.

Updated results:
https://ai.dmi.unibas.ch/_tmp_files/sieverss/issue914-v3-issue914-base-issue914-v3-compare.html

I thought the suggestion was about whether all states are goal states, not
whether all goal distances are 0 (based on the title and first message). That
doesn't require the distances.

> but I think it
> would also be easy enough to just keep track of this when transformations are
> applied.
It is certainly possible, but I wouldn't call it an "easy" change, since we
would need to keep track of two things: for each factor, we need to know if its
corresponding M&S table contains at least one "undefined value" or not, and we
would  need to know if all (other, explicitly represented) states have heuristic
value of 0 or not. 

An additional complication arises from the fact that computing goal distances is
only done if the merge or shrink strategy requires it. Otherwise, goal distances
are only computed in the end for the final heuristic. In the latter case, we
would need to determine for each factor if it is trivial or not in end anyway. 

So I think I would go with determining this information in the end only.

> To semantically determine whether a M&S representation maps everything
> to 0, we would need to make sure that no state is pruned, which would include
> any component representation. Do you consider doing this computation on demand
> an option?

Going over the tables on-demand would not be terribly expensive, but I think it
would also be easy enough to just keep track of this when transformations are
applied.

I agree with all points.

I actually like the idea of having something like "trivial" at the M&S
representation instead of the transition system. More generally, we could move
the fields "int num_variables" and "vector<int> incorporated_variables" from the
transition system to the M&S representation, since the M&S representation is the
only place where we need to know something about variables also conceptually
(and not only for printing).

Regarding the third point: I understand the concern but I don't know how we can
avoid it. To semantically determine whether a M&S representation maps everything
to 0, we would need to make sure that no state is pruned, which would include
any component representation. Do you consider doing this computation on demand
an option?

The code doesn't look pretty to me, I'm not sure I like the ratio of code
complexity vs. utility. What I don't like:

- A long method with a very generic name. Could be improved by extracting the
new code into a method with a more telling name.

- New code depends on the planning task representation (TaskProxy), where our
general strategy is "convert to the M&S representation initially, then do
everything base on that." I'd be happier if the information were tracked within
the M&S representation, for example by factors keeping track if they are "trivial".

- Related to this, this is now a syntactic criterion rather than a semantic one,
which runs counter to the M&S spirit. We can have a zero heuristic due to
shrinking without the new code detecting it.

PR is updated, performance didn't change:
https://ai.dmi.unibas.ch/_tmp_files/sieverss/issue914-v2-issue914-base-issue914-v2-compare.html

In the extreme case, we could prune all non-goal states and only keep goal
states, in which the implementation would be wrong because the heuristic would
also have infinity values. For the same reason, just checking if all non-pruned
states have heuristic values of 0 won't work. So I think the only way is to look
into the variables they factor represents. I'll change the implementation
accordingly.

Does having only goal states imply that the heuristic value is 0?
How/where is the case handled where a state is mapped to nothing because we know
it's dead?

Jendrik, could you maybe have a look? The diff is small :-)

Oups, wrong link:
https://bitbucket.org/SilvanS/fd-dev/pull-requests/49/issue914/diff

Depends on how much it complicates the code. :-) Can you link a pull request?

It is indeed a small, nearly unnoticeable change (I was just curious):
https://ai.dmi.unibas.ch/_tmp_files/sieverss/issue914-v1-issue914-base-issue914-v1-compare.html

PR:
https://ai.dmi.unibas.ch/_tmp_files/sieverss/issue914-v1-issue914-base-issue914-v1-compare.html

Any objections to merging this?

When using a time limit on the merge-and-shrink main loop, the remaining factors
are used in a maximum heuristic. As a small improvement of efficiency, we can
exclude factors that consist exclusively of goal states.