Issue526

Title use shared_ptr for Heuristics
Priority feature Status resolved
Superseder Nosy List augusto, guillem, jendrik, malte, manuel
Assigned To guillem Keywords
Optional summary

Created on 2015-04-28.10:51:53 by pvonreth, last changed by jendrik.

Messages
msg7610 (view) Author: jendrik Date: 2018-09-19.20:23:43
I created issue834 for the tasks in msg7602.
msg7602 (view) Author: malte Date: 2018-09-19.19:20:46
Great! :-) In a separate issue (which someone needs to create), we can now
remove the traces of the old class Plugin from the code. I suggest doing this in
the following steps, which in my opinion can be done in the same issue, but I
would do them in separate commits:

1. Remove code related to the class Plugin.
   I think all this code should be in the option parser subdirectory.
   It is not enough to search for "Plugin". Also look for overloads of functions
   or templates that have a version for shared pointers and another one for
   raw pointers. Searching for the word "shared" in the option parser should
   probably find most or all relevant places: the code for raw pointers should
   be right next to the code for shared pointers.

2. Optionally, clean up the method names etc. in the option parser that
   previously included the word "shared" to distinguish them from the
   raw pointer versions.

3. Rename the class PluginShared to Plugin. (This will touch lots of source
   files, so should be a commit of its own.)

There is alread a task on the taskboard for this step, but I think there isn't
an issue yet.
msg7594 (view) Author: manuel Date: 2018-09-19.17:32:06
Merged and pushed.
msg7582 (view) Author: malte Date: 2018-09-19.16:14:24
The behaviour on nomystery-sat11, instance p06, looks odd for lama-lazy. But I
think this is not related to this issue, and I'm happy to see this merged.
msg7578 (view) Author: manuel Date: 2018-09-19.15:54:04
I run experiments on configurations
- ehc with ff and ff's preferred operators,
- lama-first, and
- lama-first with eager greedy instead of lazy greedy.

You find the result here:
https://ai.dmi.unibas.ch/_tmp_files/heusner/issue526-v1-sat-issue526-base-issue526-v1-compare.html

The results show no unexpected artifacts.
msg7559 (view) Author: jendrik Date: 2018-09-19.11:17:10
I had a look and left two minor comments.
msg7540 (view) Author: malte Date: 2018-09-18.19:29:28
Done reviewing, looks good! As commented on bitbucket, I think I found a subtle
bug where preferred operators in EHC are ignored.

These sorts of things creep in easily, so I think it would be better if we ran
some experiments to make sure we didn't overlook anything.

5-minute experiments should be enough, but they should exercise all three search
algorithms (lazy, greedy, ehc), and they should include cases with preferred
operators from more than one heuristic because these are the ones most affected
by the change.

I think it's enough to do satisficing experiments. lama-first is a good test for
lazy; one of the non-lazy configs of lama is a good test for greedy. For ehc
just use one configuration with ehc, for example using the FF heuristic both as
a heuristic and for preferred operators.

Once this is merged, I'd be interested in seeing how many parts of the planner
still use "new" and/or "delete" explicitly. We should be well on our way towards
our goal of having well-defined lifetimes for everything.
msg7531 (view) Author: jendrik Date: 2018-09-18.18:17:55
Pull request: https://bitbucket.org/gutoblaas/downward-issue526/pull-requests/1
msg7505 (view) Author: guillem Date: 2018-09-18.10:55:43
I had a look at the PR; there are a few calls to 
EvaluatorContext::get_evaluator_value(evaluator.get()). Once we start moving to smart 
pointers, I'd personally try to avoid moving the same pointer around both as a "smart" and 
"non-smart" version *at the same time*, as it confuses pointer ownership and can lead to 
obscure bugs (everything is fine with this code, just thinking ahead). In this case, we 
could perhaps refactor the "get_evaluator_value" to accept a Evaluator&, and only in the 
EvaluatorCache convert that into a pointer?  (one could also use a map of 
std::reference_wrapper, but we probably don't want to pay the overhead in this particular 
place of the code).

Just a suggestion to think about!
msg7481 (view) Author: manuel Date: 2018-09-17.14:12:56
We will use shared_ptr for all plugins.
msg6929 (view) Author: jendrik Date: 2018-03-16.19:25:26
It's probably best to wait with moving forward here until issue766 and issue718 
are merged, since they touch the same portions of code.
msg6800 (view) Author: malte Date: 2018-02-09.16:25:38
Is there still a difference between Heuristic and Evaluator? If yes, then the
issue for unifying them is still open, and working on that issue at the same
time as this one will likely lead to substantial merge conflicts.

I would recommend having a brief look at the status of the Heuristic/Evaluator
unification issue before proceeding here.
msg6799 (view) Author: guillem Date: 2018-02-09.16:23:02
I'd like to push this one forward. As per the FD meeting yesterday, there should 
be no big obstacle, so I'll probably have a go at this in the next days. Although 
not mentioned in the title, I assume I could tackle using shared_ptr's for 
Evaluators as well?
History
Date User Action Args
2018-09-19 20:23:43jendriksetmessages: + msg7610
summary: Once this issue is done, rename PluginShared to Plugin. ->
2018-09-19 19:20:46maltesetmessages: + msg7602
2018-09-19 17:32:06manuelsetstatus: in-progress -> resolved
nosy: + augusto
messages: + msg7594
2018-09-19 16:14:24maltesetmessages: + msg7582
2018-09-19 15:54:04manuelsetmessages: + msg7578
2018-09-19 11:17:10jendriksetmessages: + msg7559
2018-09-18 19:29:28maltesetmessages: + msg7540
2018-09-18 18:17:55jendriksetmessages: + msg7531
summary: Blocked by issue766 and issue718. Once this issue is done, rename PluginShared to Plugin. -> Once this issue is done, rename PluginShared to Plugin.
2018-09-18 10:55:43guillemsetmessages: + msg7505
2018-09-17 14:12:56manuelsetnosy: + manuel
messages: + msg7481
2018-04-04 10:58:15jendriksetsummary: Blocked by issue766 and issue718. -> Blocked by issue766 and issue718. Once this issue is done, rename PluginShared to Plugin.
2018-03-16 19:25:26jendriksetmessages: + msg6929
summary: Blocked by issue766 and issue718.
2018-02-09 16:25:38maltesetnosy: - pvonreth
messages: + msg6800
2018-02-09 16:23:02guillemsetassignedto: pvonreth -> guillem
messages: + msg6799
nosy: + guillem
2015-04-28 10:51:53pvonrethcreate