Issue718

Title Merge Heuristic into Evaluator
Priority meta Status in-progress
Superseder Nosy List jendrik, malte, manuel
Assigned To manuel Keywords
Optional summary
Note: one issue that makes the unification harder is issue198.

Created on 2017-04-25.14:32:19 by manuel, last changed by jendrik.

Summary
Note: one issue that makes the unification harder is issue198.
Messages
msg6333 (view) Author: jendrik Date: 2017-05-03.17:02:55
I'm happy with the pull request now :-)
msg6322 (view) Author: malte Date: 2017-05-02.18:59:22
Once both of you are happy with the pull request, please let me know and I'll
also have a final look.
msg6321 (view) Author: jendrik Date: 2017-05-02.16:51:50
Sounds good to me. Maybe it would be good to factor out the common casting code.
msg6319 (view) Author: manuel Date: 2017-05-02.16:17:34
For EnforcedHillClimbingSearch there is no reason to do the casts.

For greedy and lazy searches, we have to cast the preferred operator heuristics
when they are collected to a set of heuristics. The set of heuristics consist of
pointers to heuristics because they provide the notify functions. During our
live discussions, we decided to leave the notify functions in Heuristic (not to
move them to Evaluator) and to change the subscription/notification mechanism
with issue724.

I will move the casts to the lines where they are required and change the type
of variables where possible, ok?
msg6318 (view) Author: jendrik Date: 2017-05-02.15:46:20
I added some comments on Bitbucket.

Why do we need all the dynamic_casts? Is it just to avoid changing the types of 
variables? I think that removing the casts and changing the types would yield 
clearer code.
msg6314 (view) Author: manuel Date: 2017-05-02.12:50:54
I created a new branch issue718-backup out of branch issue718. Afterwards, I
closed branch issue718 and I created a new branch with the same name out of
default. To that new branch, I imported patches that contain the meaningful
changes from the old issue718 branch.

In the new branch the changes affect options only. The type of member variables
are not changed from Heuristic to Evaluator. Also the functions are not moved
from Heuristic to Evaluator. Instead, I chose to cast Evaluator to Heuristics if
necessary and call an InputError if the Evaluator that is passed through options
is not a Heuristic. For example the InputError is called if someone puts an
Evaluator as preferred operator heuristic because an Evaluator does not provide
preferred operators. 

After our live discussions I decided for these changes to the code, because they
seemed most minimalistic and safe to me.

A new pull request can be found here:
https://bitbucket.org/manuel_h/downward/pull-requests/4/issue718/diff
msg6237 (view) Author: manuel Date: 2017-04-27.10:03:53
Here it is: https://bitbucket.org/manuel_h/downward/pull-requests/3/issue718/diff
msg6233 (view) Author: malte Date: 2017-04-26.19:11:48
Can you add a link to the pull request?

Regarding the five items, I suggest we discuss them live tomorrow.

Regarding 1), we should probably discuss with the others what we want the
command-line option and the wiki page to be called, and we should discuss the
question of whether to add sections or some other way of structuring wiki pages
with lots of entries, which we will get after merging the ScalarEvaluator and
Heuristic pages.

Regarding 2), I think we should look at where and how these descriptions are
used first. I think we currently have two kinds of descriptions; the shorter
ones that are used as "tags" in statistics and are created automatically from
the option parser, and longer ones that are used to describe to the user what is
going on. For the shorter ones, I think we can do better than we currently do
when we improve the option parser, and with the longer ones, we should probably
come up with a clearer plan of what we want to do with them. At the moment I
think it's a good idea to have proper descriptions for everything, but it would
be good to think about this feature more generally.

Regarding 4), forgetting to override this function in cases where it must be
overridden will lead to serious bugs, and in such cases our usual policy is not
to provide a default implementation because then people implementing derived
classes often don't think about whether they have to provide an implementation.
But, looking at its documentation, I think this function should probably go away
anyway in the medium term.
msg6229 (view) Author: manuel Date: 2017-04-26.17:34:58
I put a pull request on bitbucket. Up to now, I did the minimal changes to
eliminate Heuristic from plugins. During the process of this change I stumbled
over following issues:

1. Change predefinition option --heuristic to --evaluator.

2. Enable description for all evaluators, not only for heuristics. The
description for FFHeuristic is "ff", which is the plugin key. At the moment
Evaluator provides the function get_description() (which was formerly a function
of Heuristic only). It returns "n.a." for classes that do not inherit from
Heuristic.

3. Get rid of dynamic_cast for checking if Evaluator is a Heuristic. The cast
ensures that caching and counting evaluations is done for Heuristic only. I
suggest to add following functions to Evaluator: bool
cache_evaluation_results(), bool count_evaluations().  

4. Classes that inherit from Evaluator are forced to implement the virtual
function get_involved_heuristics(). Consequently, each evaluator must define it.
I suggest to define the function in Evaluator with an empty body ({}). Then, the
classes are free to redefine the function.

5. ConstEvaluator can inherit from Evaluator, because the requirement that a
search needs at least one Heuristic does not hold anymore.

I am not sure if we want to cover some of these issues now, or add them to the
issue tracker.
msg6216 (view) Author: malte Date: 2017-04-25.14:46:45
I would describe it a bit differently: we want all "user code" to use the class
Evaluator, but the class Heuristic will still exist to be used as a common base
class of specific heuristics like FFHeuristic etc. It will keep responsibilities
such as keeping track of the heurisitic cache, for example.
msg6215 (view) Author: manuel Date: 2017-04-25.14:32:19
After having renamed ScalarEvaluator to Evaluator in the process of merging
Heuristic and ScalarEvaluator to a new class (issue716), we merge Heuristic into
Evaluator. Eventually, the class Evaluator replaces the classes ScalarEvaluator
and Heuristic.
History
Date User Action Args
2017-05-03 17:02:55jendriksetmessages: + msg6333
2017-05-02 18:59:22maltesetmessages: + msg6322
2017-05-02 16:51:50jendriksetmessages: + msg6321
2017-05-02 16:17:34manuelsetmessages: + msg6319
2017-05-02 15:46:20jendriksetmessages: + msg6318
2017-05-02 12:50:54manuelsetmessages: + msg6314
2017-04-28 11:45:27jendriksetsummary: Note: one issue that makes the unification harder is issue198.
2017-04-27 10:03:53manuelsetmessages: + msg6237
2017-04-26 19:11:48maltesetmessages: + msg6233
2017-04-26 17:34:58manuelsetmessages: + msg6229
2017-04-26 09:35:33manuelsetstatus: chatting -> in-progress
2017-04-25 14:46:45maltesetmessages: + msg6216
2017-04-25 14:32:19manuelcreate