- When merging, open follow-up issue to discuss if non-perfect shrinking of
merge candidates is reasonable, and if so, at which place. (Currently, we
imitate the behavior of the merge-and-shrink computation.) See also 3) in msg6218).
- Before merging: re-order some of the methods in the implementation file of FTS
to match the order in the header file.

Created on 2016-09-02.10:46:17 by silvan, last changed by silvan.

Summary

- When merging, open follow-up issue to discuss if non-perfect shrinking of
merge candidates is reasonable, and if so, at which place. (Currently, we
imitate the behavior of the merge-and-shrink computation.) See also 3) in msg6218).
- Before merging: re-order some of the methods in the implementation file of FTS
to match the order in the header file.

> Unless you want the same old review again, it should be a new entry. :-)
Done.
> I'll try to get to it soon (perhaps out of order).
That would be great because it would ease (re-)running experiments for the
"state of the art in merge-and-shrink" for the journal paper (and thesis).

The last experiments are based on a partial integration of branch issue707, and
I don't think we changed anything else than documentation/cleaning-up in
issue707 afterwards. So yes, this issue should be ready to be reviewed and the
results are up to date.

Issue707 has been merged, and now this pull-request is up-to-date again:
https://bitbucket.org/SilvanS/fd-dev/pull-requests/22/issue668/diff
I addressed the comments of the first review where possible, and left the
discussion of if, how and when to shrink the copied transition systems when
computing their product to a future issue (or to further research on this strategy).

No, you already once looked at the code and we discussed how to improve over the
"make copies of transition systems within the FTS"-business. Once issue707 is
integrated, the computation of this strategy will be very easy, because we can
apply shrink and prune transformations also without having the copied factors
within an FTS. So once you are happy with issue707, this one will merit another
hopefully quick review.

I noticed "a bug" in the implementation of dyn-MIASM as we have been using it
since the ICAPS16 paper : the score (= ratio of alive states/total states) was
computed based on a different value for total states x than intended: x =
factor1.size * factor2.size for the unmodified factors factor1 and factor2,
rather than x = shrunk_factor1.size * shrunk_factor2.size. Recall that in the
current implementation, we shrink the temporarily copied factors using the same
strategy as the real merge strategy would use before merging them. I put bug in
quotation marks here because a) in the description of dyn-MIASM we never
mentioned that we perform shrinking before merging as usual when attempting to
compute all candidate merges, and b) you already wrote that you find it rather
unintuitive that we shrink these temporary copied factors at all.
Long story short, fixing this bug (v3) has quite an impact (comparison of v2
with v3):
http://ai.cs.unibas.ch/_tmp_files/sieverss/issue668-compare-v2-v3-abp.htmlhttp://ai.cs.unibas.ch/_tmp_files/sieverss/issue668-compare-v2-v3-pba.html
subset of the paper configs:
http://ai.cs.unibas.ch/_tmp_files/sieverss/issue668-compare-v2-v3-paper.html
Some of the configurations experience a huge increase in coverage, others a
non-negligible decrease. In particular, the better configurations now surpass
the "real MIASM" by quite a margin.
As you wrote in msg6218, I think we should first integrate the implementation
with the "bug" and then in a follow-up issue consider changing/improving the
strategy even further.

As we also need to be able to prune a transition system independently of it
being part of an FTS, we should wait with continuing with this issue until
issue707 has been merged, where we make computation of distances and pruning
configurable, and also will make the prune transformation be part of the public
interface of FTS.

Thanks! I'm done with my comments. There are three kinds of comments:
1) Small things that don't affect behaviour like documentation, typos etc.
2) The question how exactly we want to implement the "temporary merges".
3) The question which shrink strategy should be used within MIASM.
Dealing with 1) should not be too difficult. Before merging, I think we should
address 2) too; I think the current solution is a bit too hacky because it
allows leaving the FTS in an invalid state, which I think always increases the
"mental load" for understanding the code a lot.
Regarding 3), perhaps we need to discuss this conceptually and perhaps also run
some additional experiments. I think this is something that we could potentially
defer to a follow-up issue.

Hi Silvan, it's FIFO time! This one comes next after issue667.
Can you bring the pull request up to date, i.e., make sure that it is based on
the most recent master version? If I look at it right now, I see some small
conflicts (e.g. in the CMake files).

I ran experiments, including full tie-breaking variants as described in the
ICAPS2016 paper.
http://ai.cs.unibas.ch/_tmp_files/sieverss/issue668-v1-abp.htmlhttp://ai.cs.unibas.ch/_tmp_files/sieverss/issue668-v1-pba.html
abp: atomic before product (prefer atomic transition systems over products)
pba: product before atomic (prefer product transition systems over atomics)
rl/l/rnd: order of atomic transition systems (reverse level/level/random)
nto/otn/rnd: order product transition systems (new to old/old to new/random)
This table is just a subset of the above configurations, matching Table 3 of the
paper:
http://ai.cs.unibas.ch/_tmp_files/sieverss/issue668-v1-paper.html
There are quite some differences in the configurations involving randomness. I
suspect that this has to do with the change of how we use random number
generators (the old global one, or a local one, or a mixture). Another
difference to the paper version is that back then, when temporarily merging
entries of the factored transition system, we did not only copy the transition
systems, but also the merge-and-shrink representation. As this is not necessary,
we do not do it anymore.

I made some preliminary comments on bitbucket, but I realized I cannot really
review this now or probably in the next days. Can you please insert it into the
(to be discussed) queue? Then I'll pop it from my inbox for now.

> I like the idea of such a queue, do you have a good idea where it could live,
> so that you could easily administrate it and we could see the status?
I don't know. Maybe something to discuss for the next Fast Downward meeting?

naming it "verbose" to have the positive use case in the code. (My touchpad just
hit the send button...)
I like the idea of such a queue, do you have a good idea where it could live, so
that you could easily administrate it and we could see the status?

> As for issue667, I'd like to queue this code review into your list,
> but I guess issue667 would be much easier to be integrated first.
Perhaps it would be a good idea to actually start such a review queue, not just
during the sprints? It would help me keeping track of things, and it might also
be useful for the others to see what is happening.

I had a quick look, but it's a bit much to review quickly in one chunk, and I
think the changes to the "infrastructure" are large enough that a review would
be good.
Two comments:
1. I think we could speed up the review process by separating the change with
the "silent" option (conceptually simple, but many lines and many files
affected) from the other changes related to copying stuff. What do you think? It
would be extra work for you, but it might get merged faster overall.
2. I wonder if we should call the option "verbose" rather than "silent" because
we always test for the negated form ("!silent"). I searched the code for
precedent on this, but I found no occurrences of either "silent" or "verbose".

For the temporary computation of the products corresponding to all merge
candidates, the new code requires quite some changes to the existing code:
- Because we want to apply shrinking before merging, we need to actually copy
the elements of the factored transition system (i.e. transition
system/distances/heuristic representation) before doing so, to not compromise
the rest of the factored transition system. This requires copy constructors in
several classes, and also to have the factored transition system qualified
non-const.
- Because merge-and-shrink usually prints logging messages when
shrinking/merging/pruning, we need to disable such logging whenever operating on
the copied objects, to avoid producing very large log files. This requires
passing around "silent" flags in several places.
- To be able to use the same shrinking settings for the new scoring function, it
needs to have the exact same shrinking related options as
MergeAndShrinkHeuristic itself. This means to have small duplication in the
command line syntax (and of course a slight memory overhead for storing a second
shrink strategy, but these objects should be very light weight).
All of these issues have been part of the strategy already when we described in
the paper, so there are no performance concerns, but code quality concerns.
As for issue667, I'd like to queue this code review into your list, but I guess
issue667 would be much easier to be integrated first.
https://bitbucket.org/SilvanS/fd-dev/pull-requests/20/issue668/diff

We would like to integrate the recent merge strategy "dynamic MIASM", which is a
dynamic, i.e. non-precomputed, variant of the original MIASM merge strategy,
described in the ICAPS 2016 paper An Analysis of Merge Strategies for
Merge-and-Shrink Heuristics.
Technically speaking, this means to "just" add a new merge scoring function
which prefers merges that maximize pruning opportunities through throwing away
unreachable and irrelevant states.

History

Date

User

Action

Args

2017-07-14 16:09:24

silvan

set

summary: - When merging, open follow-up issue to discuss if non-perfect shrinking of
merge candidates is reasonable, and if so, at which place. (Currently, we
imitate the behavior of the merge-and-shrink computation.) See also 3) in msg6218). -> - When merging, open follow-up issue to discuss if non-perfect shrinking of
merge candidates is reasonable, and if so, at which place. (Currently, we
imitate the behavior of the merge-and-shrink computation.) See also 3) in msg6218).
- Before merging: re-order some of the methods in the implementation file of FTS
to match the order in the header file.

messages:
+ msg6429summary: - When merging, open follow-up issue to discuss which shrink strategy should be
used when shrinking the temporary copies of transition systems (see 3) in msg6218). -> - When merging, open follow-up issue to discuss if non-perfect shrinking of
merge candidates is reasonable, and if so, at which place. (Currently, we
imitate the behavior of the merge-and-shrink computation.) See also 3) in msg6218).