Title Use OperatorID instead of GlobalOperator* more often
Priority wish Status resolved
Superseder Nosy List florian, jendrik, malte
Assigned To florian Keywords
Optional summary

Created on 2017-04-28.19:33:35 by florian, last changed by florian.

msg6580 (view) Author: florian Date: 2017-11-02.15:15:44
Merged. Thanks for the review.
msg6579 (view) Author: malte Date: 2017-11-02.15:00:35
Pull request and experiments look good. Once this is integrated with the new
code, I think it can be merged without further review or experiments.
msg6331 (view) Author: florian Date: 2017-05-03.11:20:55
I ran A* with the blind and LM-cut heuristics for the optimal benchmark
and lama, lama-first, and ehc with the FF heuristic for the satisficing
benchmarks. The coverage is unaffected in all configurations.

The optimal results surprised me a little. I expected runtime to go up
slightly in both cases and expected the blind search to be affected
more because LM-cut spends more time in the heuristic computation.
Instead, the times for blind search actually improved by a few percent.
Its less than the typical noise but clearly visible in the plot. The
runtime of LM-cut on the other hand increased as predicted.

Both effects are small enough (2-3%) that I think we can live with them
and merge this isue.

For the satisficing results, I only generated the plot for lama-first
and ehc because the timing results of lama depend on the last
successful iteration. The noise is larger in satisficing benchmarks as
usual and I don't see a large impact here.
msg6317 (view) Author: malte Date: 2017-05-02.15:35:37
LAMA (not lama-first) is a good configuration for "iterated". For EHC, just pick
anything that we've used in experiments in the past. In older versions of lab, I
recall there were some "extra default configurations" that included some variant
of EHC, but I don't recall the details.
msg6316 (view) Author: florian Date: 2017-05-02.15:32:05
Thanks Jendrik. If you have any more comments, we can fix them before this issue
reaches the top of Malte's queue (I added it now).

What kind of experiments should I run? lama-first would cover a lot of changes
(LazySearch, EdgeOpenList, notify_state_transition, the global changes like
using ID in search node info, saving plans and computing their costs). That
leaves the other search methods (eager, EHC, iterated). I could do A*/LM-cut for
eager but what are good configurations for EHC and iterated?
msg6313 (view) Author: malte Date: 2017-05-02.11:51:56
Hi Florian, if you also want me to take a look at this, please add it to the
review queue.
msg6297 (view) Author: jendrik Date: 2017-04-30.18:29:22
I made some minor comments on Bitbucket.
msg6296 (view) Author: jendrik Date: 2017-04-30.17:04:56
I added a discussion about real_g to the agenda.
msg6294 (view) Author: florian Date: 2017-04-29.02:10:12
I created a pull request for the code I would like to change. The major changes are:
 * Plans (was: vector<const GlobalOperator *> now: vector<OperatorID>)
 * notify_state_transition (was: const GlobalOperator * now: OperatorID)
 * EnforcedHillClimbingSearch::reach_state and 
 * EdgeOpenListEntry (was: pair<StateID, int> now: pair<StateID, OperatorID>)
 * EnforcedHillClimbingSearch::insert_successor_into_open_list (parameter was: 
   const GlobalOperator * now: OperatorID) 
 * LazySearch::current_operator (was: const GlobalOperator * now: OperatorID)
 * SearchNodeInfo::creating_operator (was: int now: OperatorID)
 * A couple of removed unused forward declarations

Let me know if you prefer some of these changes in their own issue.
msg6290 (view) Author: malte Date: 2017-04-28.22:54:00
I think both of them should disappear from SearchNodeInfo eventually -- not
least because many configurations don't need them, and they waste memory. In
configurations without a passed-in cost bound, certainly we don't need two g
values. And in non-reopening greedy search configurations without a passed-in
bound (e.g. lama-first, or more generally most lazy_greedy configurations), we
need neither.

Regarding the way ahead with this issue, I suggest you repurpose it for whatever
you want to integrate right now. No need to open a separate issue.
msg6289 (view) Author: florian Date: 2017-04-28.22:48:44
In SearchNodeInfo, we store both values, so we currently need access to both. If
one of them would disappear into the evaluator, that would help. The change in
perspective sounds good for that.

But it also sounds like a larger operation. I mainly started this issue to get
some quick fixes and cleanup commits in that are possible now that we have
operator IDs. The other changes I made so far still look good to me. Should I
open a new issue for them, so we can merge them?
Either way, I suggest we put this one on ice until we figure out how to handle
real_g. Jendrik, can you put this on the agenda for the next meeting, please?
msg6288 (view) Author: malte Date: 2017-04-28.22:28:53
Are you working from the assumption that the "normal" task that the search
algorithm operates on is the one that provides "g", and we need some special
mechanism to get at "real_g" because it might differ from "g"?

If yes, it might be possible to shift perspective and make the task that
provides "real_g" the "normal" one from the perspective of the search algorithm,
and "g" is based on a GEvaluator operating on a transformed task. (I think
GEvaluators currently cannot work on transformed tasks because task
transformations are introduced further down in the ScalarEvaluator/Heuristic
hierarchy, but this is something we could change.)

Would this help?

The point is that there are not so many things that "g" is actually used for
during search, and it might be quite possible to make "g" just another aspect of
the evaluator that is used within the open list.
msg6287 (view) Author: florian Date: 2017-04-28.21:57:56
Ahh, this might not so easy as long as SearchNodeInfo still stores real_g. We
can only get this cost from the GlobalOperator, so we cannot pass a
OperatorProxy to SearchNode::open and similar functions. We could pass in a
GlobalOperator and an OperatorID but that feels like adding one hack to get rid
of another.
msg6281 (view) Author: florian Date: 2017-04-28.19:33:35
Now that issue688 is merged, we can start to clean up some transitional code.
In this issue, I would like to get rid of get_op_index_hacked which looks up the
ID of a global operator. It is currently used by all three searches in
situations where the ID should be available.

In this context, I would like to use OperatorID instead of int for
SearchNodeInfo::creating_operator and EdgeOpenListEntry, the two places where
the function is used.
Date User Action Args
2017-11-02 15:15:44floriansetstatus: chatting -> resolved
messages: + msg6580
2017-11-02 15:00:35maltesetmessages: + msg6579
2017-05-03 11:20:55floriansetmessages: + msg6331
2017-05-02 15:35:37maltesetmessages: + msg6317
2017-05-02 15:32:05floriansetmessages: + msg6316
2017-05-02 11:51:56maltesetmessages: + msg6313
2017-04-30 18:29:22jendriksetmessages: + msg6297
2017-04-30 17:04:56jendriksetmessages: + msg6296
2017-04-29 02:10:12floriansetmessages: + msg6294
title: Get rid of get_op_index_hacked -> Use OperatorID instead of GlobalOperator* more often
2017-04-28 22:54:00maltesetmessages: + msg6290
2017-04-28 22:48:44floriansetmessages: + msg6289
2017-04-28 22:28:53maltesetmessages: + msg6288
2017-04-28 21:57:56floriansetstatus: unread -> chatting
messages: + msg6287
2017-04-28 19:33:35floriancreate