Issue718

Title use Evaluator instead of Heuristic in option parser
Priority feature Status in-progress
Superseder Nosy List guillem, jendrik, malte, manuel
Assigned To manuel Keywords
Optional summary
This is part of issue727. Currently, it is blocked by issue766, issue724 and
issue750.

While the original scope of this issue was broader, the new goal is to use only 
"Evaluator" and not "Heuristic" in the option parser code. This means that all 
current "Heuristic" plugins need to return "Evaluator" pointers. Before we 
tackle 
this issue, we need to make sure that all public methods of the "Heuristic" 
class 
are also part of the "Evaluator" class. issue724 deals with the "notify_*" 
methods and the "get_involved_heuristics()" method. The last remaining public 
method that needs to be promoted to the "Evaluator" class is 
"get_description()".

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

Summary
This is part of issue727. Currently, it is blocked by issue766, issue724 and
issue750.

While the original scope of this issue was broader, the new goal is to use only 
"Evaluator" and not "Heuristic" in the option parser code. This means that all 
current "Heuristic" plugins need to return "Evaluator" pointers. Before we 
tackle 
this issue, we need to make sure that all public methods of the "Heuristic" 
class 
are also part of the "Evaluator" class. issue724 deals with the "notify_*" 
methods and the "get_involved_heuristics()" method. The last remaining public 
method that needs to be promoted to the "Evaluator" class is 
"get_description()".
Messages
msg6910 (view) Author: manuel Date: 2018-03-16.15:41:40
Update summary.
msg6833 (view) Author: guillem Date: 2018-03-14.17:44:15
Just a quick note: issue724 is merged into master, we can have a look together at 
this one tomorrow, Manuel, and see if I can help with it.
msg6651 (view) Author: malte Date: 2017-11-30.12:20:07
This is currently at the top of the review queue, but we cannot continue with it
at the moment because it is blocked. I looked at issue750 instead and will
remove this from the review queue for now.
msg6639 (view) Author: jendrik Date: 2017-11-29.23:26:14
Update summary.
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
2018-03-16 15:41:40manuelsetmessages: + msg6910
summary: This is part of issue727. Currently, it is blocked by issue724 and issue750. While the original scope of this issue was broader, the new goal is to use only "Evaluator" and not "Heuristic" in the option parser code. This means that all current "Heuristic" plugins need to return "Evaluator" pointers. Before we tackle this issue, we need to make sure that all public methods of the "Heuristic" class are also part of the "Evaluator" class. issue724 deals with the "notify_*" methods and the "get_involved_heuristics()" method. The last remaining public method that needs to be promoted to the "Evaluator" class is "get_description()". -> This is part of issue727. Currently, it is blocked by issue766, issue724 and issue750. While the original scope of this issue was broader, the new goal is to use only "Evaluator" and not "Heuristic" in the option parser code. This means that all current "Heuristic" plugins need to return "Evaluator" pointers. Before we tackle this issue, we need to make sure that all public methods of the "Heuristic" class are also part of the "Evaluator" class. issue724 deals with the "notify_*" methods and the "get_involved_heuristics()" method. The last remaining public method that needs to be promoted to the "Evaluator" class is "get_description()".
2018-03-14 17:44:15guillemsetmessages: + msg6833
2017-11-30 12:20:07maltesetnosy: + guillem, - gfrances
messages: + msg6651
2017-11-29 23:26:14jendriksetmessages: + msg6639
summary: While the original scope of this issue was broader, the new goal is to use only "Evaluator" and not "Heuristic" in the option parser code. This means that all current "Heuristic" plugins need to return "Evaluator" pointers. Before we tackle this issue, we need to make sure that all public methods of the "Heuristic" class are also part of the "Evaluator" class. issue724 deals with the "notify_*" methods and the "get_involved_heuristics()" method. The last remaining public method that needs to be promoted to the "Evaluator" class is "get_description()". -> This is part of issue727. Currently, it is blocked by issue724 and issue750. While the original scope of this issue was broader, the new goal is to use only "Evaluator" and not "Heuristic" in the option parser code. This means that all current "Heuristic" plugins need to return "Evaluator" pointers. Before we tackle this issue, we need to make sure that all public methods of the "Heuristic" class are also part of the "Evaluator" class. issue724 deals with the "notify_*" methods and the "get_involved_heuristics()" method. The last remaining public method that needs to be promoted to the "Evaluator" class is "get_description()".
2017-11-29 15:39:09jendriksetnosy: + gfrances
title: Merge Heuristic into Evaluator -> use Evaluator instead of Heuristic in option parser
summary: Note: one issue that makes the unification harder is issue198. -> While the original scope of this issue was broader, the new goal is to use only "Evaluator" and not "Heuristic" in the option parser code. This means that all current "Heuristic" plugins need to return "Evaluator" pointers. Before we tackle this issue, we need to make sure that all public methods of the "Heuristic" class are also part of the "Evaluator" class. issue724 deals with the "notify_*" methods and the "get_involved_heuristics()" method. The last remaining public method that needs to be promoted to the "Evaluator" class is "get_description()".
2017-11-27 11:15:10jendriksetpriority: meta -> feature
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