[issue1082] Use Options objects only in Features #218

SimonDold · 2024-02-01T11:19:48Z

No description provided.

# Conflicts: # src/search/cartesian_abstractions/utils.cc # src/search/evaluator.cc # src/search/evaluator.h # src/search/heuristic.cc # src/search/heuristic.h # src/search/heuristics/additive_heuristic.cc # src/search/heuristics/additive_heuristic.h # src/search/heuristics/blind_search_heuristic.cc # src/search/heuristics/blind_search_heuristic.h # src/search/heuristics/ff_heuristic.cc # src/search/heuristics/ff_heuristic.h # src/search/heuristics/max_heuristic.cc # src/search/heuristics/max_heuristic.h # src/search/heuristics/relaxation_heuristic.cc # src/search/heuristics/relaxation_heuristic.h

src/search/evaluator.cc

src/search/heuristic.h

src/search/utils/logging.cc

src/search/heuristics/max_heuristic.h

salome-eriksson

This is only a partial review looking at the non-mechanical changes. The second part will follow later.

misc/tests/test-parameters.py

src/search/utils/logging.h

salome-eriksson · 2024-06-28T10:54:04Z

src/search/utils/rng_options.cc

-    int seed = options.get<int>("random_seed");
+tuple<int> get_rng_arguments_from_options(
+    const plugins::Options &opts) {
+    return make_tuple(


This return statement would comfortably fit on one line, making it more readable in my opinion.

salome-eriksson · 2024-06-28T11:00:53Z

src/search/utils/tuples.h

@@ -0,0 +1,31 @@
+#ifndef UTILS_TUPLES_H


I wonder if adding some comments on how this template magic works would be beneficial. I only understood the code when Florian walked me through it. On the other hand I'm not sure if we need a detailed explanation, or are just happy that it does work ;).

I think this is hard to understand and I also only understood it when Florian explained it to me.

A permanent place to find an explanation of it would be nice but the comments might not be the right place for it, is it?

reduce empty lines.

Not using ',' anymore improve pattern

FlorianPommerening

Part 1 of my comments, I'll add the rest later.

misc/tests/test-parameters.py

FlorianPommerening · 2024-06-28T12:27:56Z

src/search/cartesian_abstractions/additive_cartesian_heuristic.cc

    shared_ptr<utils::RandomNumberGenerator> rng =
-        utils::parse_rng_from_options(opts);
+        utils::get_rng(random_seed);


Does not fit the '72 characters if possible' style guide
"Line width is 72 characters if possible, 80 characters max." - https://www.fast-downward.org/ForDevelopers/Whitespace?highlight=%2872%29

FlorianPommerening · 2024-06-28T12:30:12Z

src/search/cartesian_abstractions/additive_cartesian_heuristic.cc

+                              subtasks, max_states, max_transitions,
+                              max_time, pick, use_general_costs,
+                              random_seed, transform, log)) {


Suggested change

subtasks, max_states, max_transitions,

max_time, pick, use_general_costs,

random_seed, transform, log)) {

subtasks, max_states, max_transitions, max_time, pick,

use_general_costs, random_seed, transform, log)) {

indent with 4 spaces

i agree
but tox -e fix-style disagrees.

FlorianPommerening · 2024-06-28T12:57:37Z

src/search/cartesian_abstractions/subtask_generators.cc

+    return tuple_cat(make_tuple(opts.get<FactOrder>("order")),
+                     utils::get_rng_arguments_from_options(opts));


Suggested change

return tuple_cat(make_tuple(opts.get<FactOrder>("order")),

utils::get_rng_arguments_from_options(opts));

return utils::make_flat_tuple(

opts.get<FactOrder>("order"),

utils::get_rng_arguments_from_options(opts));

with an implementation of make_flat_tuple() in tuple.h:

template<class ... Ts> auto make_flat_tuple(Ts&&... ts) { return flatten_tuple(std::forward_as_tuple(ts...)); }

This also needs an overload of flatten_tuple for l-value reverences, because forward_as_tuple creates them. It should be doable with perfect forwarding to not duplicate the code here but I didn't manage.

template<class ... Ts> auto flatten_tuple(std::tuple<Ts...> &t) { constexpr std::size_t tuple_size = std::tuple_size<std::tuple<Ts...>>::value; return flatten_tuple_elements( std::move(t), std::make_index_sequence<tuple_size>()); }

src/search/cartesian_abstractions/utils.cc

src/search/cartesian_abstractions/utils_landmarks.cc

salome-eriksson

Most of the comments are easy fixes.
Comments about style can also be ignored if you disagree.
One recurring comment is that the order of arguments in the constructor does not match the order of fields in the class definition. I personally find it more readable if they are the same (that is, if the arguments are used in the same order in the initialization list as in the constructor arguments, minus the ones that go to the parent of course). But I'm also ok with leaving it as it is.

salome-eriksson · 2024-06-28T12:21:07Z

src/search/utils/rng_options.cc

-    int seed = options.get<int>("random_seed");
+tuple<int> get_rng_arguments_from_options(
+    const plugins::Options &opts) {
+    return make_tuple(


Also, do we really need to return a tuple here?

salome-eriksson · 2024-06-28T12:25:46Z

src/search/cartesian_abstractions/cost_saturation.h

    const PickSplit pick_split;
+    const bool use_general_costs;


Why did we change the order here?

To match the parameter order in the constructor of CostSaturation, there it was changed to match the constructor of AdditeiveCartesianHeuristic, there it was chosen in the way to match the wiki.

src/search/evaluator.h

src/search/heuristic.cc

salome-eriksson · 2024-07-02T13:43:47Z

src/search/search_algorithms/enforced_hill_climbing_search.cc

-      preferred_operator_evaluators(opts.get_list<shared_ptr<Evaluator>>("preferred")),
-      preferred_usage(opts.get<PreferredUsage>("preferred_usage")),
+    const shared_ptr<Evaluator> &h, PreferredUsage preferred_usage,
+    const vector<shared_ptr<Evaluator>> &preferred,


Field order inconsistent with constructor order.

salome-eriksson · 2024-07-02T13:47:18Z

src/search/search_algorithms/lazy_search.cc

+      randomize_successors(randomize_successors),
+      preferred_successors_first(preferred_successors_first),
+      rng(utils::get_rng(random_seed)),
+      preferred_operator_evaluators(preferred),


Field order inconsistent with constructor order (preferred_operator_evaluators)

salome-eriksson · 2024-07-02T13:51:13Z

src/search/search_algorithms/plugin_eager_greedy.cc

-        options_copy.set("open", search_common::create_greedy_open_list_factory(options_copy));
-        options_copy.set("reopen_closed", false);
-        shared_ptr<Evaluator> evaluator = nullptr;
-        options_copy.set("f_eval", evaluator);


Other times we kept using options_copy. What is the difference here? Could we also get rid of it in other cases?

We kept it if it was not trivial how to remove it.
It would be possible to get rid of more opts_copies by moving the logic into the create_component function but I think keeping it somewhat encapsulated is nice.

src/search/search_algorithms/search_common.cc

FlorianPommerening · 2024-07-02T14:08:52Z

the order of arguments in the constructor does not match the order of fields in the class definition.

Isn't this a compiler warning that we treat as an error? I guess not, since it compiles, but I thought this would be a requirement.

maltehelmert · 2024-07-02T14:21:14Z

the order of arguments in the constructor does not match the order of fields in the class definition.

Isn't this a compiler warning that we treat as an error? I guess not, since it compiles, but I thought this would be a requirement.

I think it would be good to trigger an error in the Github Actions, but not I don't think it's necessarily worth investing time to try to make that happen if it doesn't currently.

In case you don't know, unless I misremember, C++ semantics is that the initialization (including evaluation of the initializers) happens in the order in the class definition, not the order given in the constructor.

So for example, something like this:

X::X() : foo(f()), bar(g()) {}

would first call g() in order to initialize bar and then call f() in order to initalize foo if bar comes before foo in the class definition.

…ptions.

SimonDold and others added 18 commits November 22, 2023 16:44

Move unpacking of opts to BlindSearchHeuristicFeature.

c37b2c3

Florians feedback

681c3e1

add malte discussion comment

8db40f2

remove destructor.

d2403f9

add name parameter to evaluator

3eca358

Move unpacking of opts to BlindSearchHeuristicFeature.

829df0a

Florians feedback

a92b5b4

add malte discussion comment

696a2d9

remove destructor.

5428d7f

Move unpacking of opts to BlindSearchHeuristicFeature.

dac2226

Florians feedback

f6a4256

remove destructor.

e8c9874

add name parameter to evaluator

a72fffd

[issue1082] intern parameter order

3fc0abc

[issue1082] style

0e160d5

[issue1082] fix rebase

1e315dd

[issue1082] restore destructors

f03ba76

jendrikseipp requested changes Feb 1, 2024

View reviewed changes

src/search/evaluator.cc Outdated Show resolved Hide resolved

src/search/heuristic.h Outdated Show resolved Hide resolved

src/search/utils/logging.cc Outdated Show resolved Hide resolved

src/search/heuristics/max_heuristic.h Outdated Show resolved Hide resolved

SimonDold added 11 commits February 1, 2024 14:33

[issue1082] Jendriks requested changes

af6f95a

[issue1082] make transform const

4f671ae

[issue1082] add default name, adjust wiki order

1170b1e

[issue1082] style

d688a65

[issue1082] move 'name' to log_options

a6ca706

[issue1082] comments

34e63ba

[issue1082] style

87f0f8b

[issue1082] comment

50cf916

[issue1082] update merge_strategy_factory_precomputed

edc1fdd

[issue1082] update merge_strategy_factory_sccs

218c31a

[issue1082] style

6047e02

salome-eriksson reviewed Jun 28, 2024

View reviewed changes

SimonDold and others added 4 commits June 28, 2024 18:44

Update logging.h

45d501e

reduce empty lines.

style

361fbdd

implement feddback

7dd6c8d

Not using ',' anymore improve pattern

reduce error message width.

7fd9d24

FlorianPommerening reviewed Jul 1, 2024

View reviewed changes

salome-eriksson reviewed Jul 2, 2024

View reviewed changes

SimonDold added 6 commits July 2, 2024 21:41

adjust to review comments.

dd1fda2

review comments

d4f8655

style

22b3c76

remove unused function.

fd037d7

remove unused function.

2ae78b9

undo unnecessary change.

0306ecc

roeger mentioned this pull request Jul 3, 2024

Issue454 - move computation of negated axioms to search component #220

Merged

SimonDold added 13 commits July 3, 2024 20:07

adjust parameter order.

a6fc4a5

undo unnecessary include.

97a16f2

fix style

c50b1a0

use move in constructor.

03ee101

change parameter order.

a4eadf7

break ling line.

2c9f383

rename function.

f9b49dc

adjust field order to match constructor parameter order.

8f3b87b

put include back into .cc and add get_canonical_pdbs_arguments_from_o…

51e101c

…ptions.

remove unnecessary include.

1e13122

fix style.

c623d0d

merge

e228cac

add comment.

4a7cea2

SimonDold merged commit 54e8669 into aibasel:main Jul 5, 2024
11 of 12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[issue1082] Use Options objects only in Features #218

[issue1082] Use Options objects only in Features #218

SimonDold commented Feb 1, 2024

salome-eriksson left a comment

salome-eriksson Jun 28, 2024

SimonDold Jun 28, 2024

salome-eriksson Jun 28, 2024

SimonDold Jun 28, 2024

FlorianPommerening left a comment

FlorianPommerening Jun 28, 2024

SimonDold Jul 2, 2024

FlorianPommerening Jun 28, 2024

SimonDold Jul 2, 2024 •

edited

Loading

FlorianPommerening Jun 28, 2024

salome-eriksson left a comment

salome-eriksson Jun 28, 2024

salome-eriksson Jun 28, 2024

SimonDold Jul 5, 2024

salome-eriksson Jul 2, 2024

salome-eriksson Jul 2, 2024

salome-eriksson Jul 2, 2024

SimonDold Jul 5, 2024

FlorianPommerening commented Jul 2, 2024 •

edited

Loading

maltehelmert commented Jul 2, 2024

		return tuple_cat(make_tuple(opts.get<FactOrder>("order")),
		utils::get_rng_arguments_from_options(opts));

[issue1082] Use Options objects only in Features #218

[issue1082] Use Options objects only in Features #218

Conversation

SimonDold commented Feb 1, 2024

salome-eriksson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FlorianPommerening left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonDold Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salome-eriksson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FlorianPommerening commented Jul 2, 2024 • edited Loading

maltehelmert commented Jul 2, 2024

SimonDold Jul 2, 2024 •

edited

Loading

FlorianPommerening commented Jul 2, 2024 •

edited

Loading