Issue721

Title Extract parse options from lazy- and eager-search into their own files
Priority wish Status resolved
Superseder Nosy List danielk, florian, jendrik, malte
Assigned To danielk Keywords
Optional summary

Created on 2017-04-27.17:25:01 by danielk, last changed by jendrik.

Messages
msg6369 (view) Author: jendrik Date: 2017-05-15.16:50:09
Thanks, Daniel and Malte! I just merged the code.
msg6368 (view) Author: malte Date: 2017-05-15.11:43:24
I made a few comments. Looks fine to me otherwise if the tests pass and
documentation still looks the same.
msg6334 (view) Author: jendrik Date: 2017-05-03.20:55:03
Daniel and I are happy with the pull request now. You can find it at 
https://bitbucket.org/danielkillenberger/fast-downward/pull-requests/6 . Malte, 
could you have a look, please?
msg6304 (view) Author: jendrik Date: 2017-04-30.21:49:56
Sorry, I messed up the example. I meant running "eager_greedy(blind())" without 
compiling the tie-breaking open list. I'd say we don't move any functions from 
search_common to other files, since there's no clear net benefit at the moment.
msg6302 (view) Author: malte Date: 2017-04-30.21:41:45
Reducing such dependencies is one thing we like to do, but in this case, or at
least with this particular separation, I don't see a net win when also
considering other aspects.

For what it's worth, regarding the "astar(blind())" example, astar *always*
requires the tie-breaking open list, right?

Looking in more detail at how the functions in search_common.cc depend on each
other, moving create_astar_open_list_factory_and_f_eval into plugin_astar may
make some sense because it does not depend on the rest of the file (nor vice
versa) and it is the only function in the file using the tie-breaking open list.
But it can also be argued that it is useful to keep it together with the
weighted A* open list/evaluator code because they are likely to be changed together.
msg6300 (view) Author: jendrik Date: 2017-04-30.21:29:31
Thanks for clarifying. I completely agree with your points. I just understood we 
wanted to allow running, e.g., astar(blind()) without having to compile the 
tiebreaking open list. If we can't think of a clean solution for this, I agree that 
we should prefer code clarity over compile times.
msg6299 (view) Author: malte Date: 2017-04-30.21:19:13
I don't think this improves the structure. The functions in search_common.cc are
currently quite coherent: if one of these methods needs to be changed, it is
likely that the others also need to be changed because they are "about the same
thing".

The functions in search_common.cc offer a clear interface between the search
algorithms and the low-level open lists and evaluators. The factories in
search_common don't need to know anything about search algorithms, and the
search algorithms and plug-ins don't need to know anything about how to create
open lists and evaluators.

With the suggested change, creating greedy, weighted A* and A* open lists, which
are really variations of the same theme, would be in three different files with
file names that don't communicate any relationship between them and rather
suggest that they are about three different kinds of things
("open_list_factory", "lazy_search" and "plugin_astar" all seem to be on
different levels).

Also, the suggested methods for lazy_search.cc have nothing to do with lazy
search as such (we just happen not to use them for eager search at the moment),
and I think it was a gain in readability to move them out of lazy_search into a
separate file to facilitate their reuse. (If we added a "wastar" plugin, which
we really should, we'd never consider moving these files to lazy_search.)
msg6298 (view) Author: jendrik Date: 2017-04-30.18:46:11
To clarify the dependencies of the eager search plugins and the open list factories 
I think it makes sense to split up the functions in search_common.cc into separate 
files (and remove search_common.cc). What do you think about the following 
destinations?

open_list_factory.h:
  create_standard_scalar_open_list_factory
  create_alternation_open_list_factory
  create_alternation_open_list_factory_aux
  create_greedy_open_list_factory

lazy_search.cc:
  create_wastar_eval
  create_wastar_open_list_factory
  (These functions will need to go into a separate file if we ever 
   implement eager weighted A*. We might therefore also consider 
   keeping them in the search_common module.)

plugin_astar.cc:
  create_astar_open_list_factory_and_f_eval
msg6276 (view) Author: jendrik Date: 2017-04-28.18:37:28
We discussed this again and agreed on the following names (following our 
conventions):

filename: plugin_astar.cc
namespace: plugin_astar
CMake plugin: PLUGIN_ASTAR
msg6266 (view) Author: jendrik Date: 2017-04-28.11:53:27
Yes, I thought the plan was to have EAGER_SEARCH as a dependency_only plugin and 
e.g. ASTAR as a plugin that depends on EAGER_SEARCH.
msg6261 (view) Author: florian Date: 2017-04-28.11:11:57
I'm not sure if I understand exactly. Are you proposing two CMake plugins, one
for the search and one for the search plugin? Isn't that a bit much?
msg6257 (view) Author: jendrik Date: 2017-04-27.23:19:06
We want to create the following new files:

    plugin_astar.cc
    plugin_eager.cc
    plugin_eager_greedy.cc
    plugin_lazy.cc
    plugin_lazy_greedy.cc
    plugin_lazy_wastar.cc

Our usual name mapping would require to name the CMake plugins "PLUGIN_ASTAR", etc. 
and the namespaces "plugin_astar", etc. Obviously, the CMake plugin translation 
rule can only be applied if the plugin consists of one file or is the whole 
directory. Also, currently we don't always follow the rule. Applying it here would 
have the drawback that the options are called PLUGIN_PLUGIN_ASTAR in ccmake and 
IDEs. I therefore propose to name the CMake plugins "ASTAR", etc. I'm not so sure 
about the namespace names, but I lean towards "plugin_astar" instead of "astar". 
What do you think?
msg6248 (view) Author: malte Date: 2017-04-27.17:50:05
IIRC, we generally have a 1:1 mapping between CMake plugins and namespaces (with
some exceptions for things like the core). So if we put them into their own
CMake plug-ins, they should have their own namespaces. (And I think it's useful
to put them into their own CMake plugins if they can be enabled/disabled
selectively.)

There is also a mechanical/algorithmic mapping between the names. It would be
good to preserve this, because it reduces the number of decisions we need to make.

Florian or Jendrik probably know better than me if this is true and what the
corresponding names should be.
msg6246 (view) Author: danielk Date: 2017-04-27.17:25:01
Extract different parse options from lazy- and eager-search into their own files and 
give them their own plugins. Are individual namespaces required?
History
Date User Action Args
2017-05-15 16:50:09jendriksetstatus: in-progress -> resolved
messages: + msg6369
2017-05-15 11:43:25maltesetmessages: + msg6368
2017-05-03 20:55:03jendriksetmessages: + msg6334
2017-04-30 21:49:56jendriksetmessages: + msg6304
2017-04-30 21:41:45maltesetmessages: + msg6302
2017-04-30 21:29:31jendriksetmessages: + msg6300
2017-04-30 21:19:13maltesetmessages: + msg6299
2017-04-30 18:46:11jendriksetmessages: + msg6298
2017-04-28 18:37:28jendriksetmessages: + msg6276
2017-04-28 11:53:27jendriksetmessages: + msg6266
2017-04-28 11:11:57floriansetmessages: + msg6261
2017-04-27 23:19:06jendriksetmessages: + msg6257
2017-04-27 17:50:05maltesetmessages: + msg6248
2017-04-27 17:25:01danielkcreate