issue992 #246

ClemensBuechner · 2025-02-04T09:34:49Z

No description provided.

SimonDold

I left a few comments, nothing big.
The biggest surprise to me was that the landmarkgraph is now treated as an iterable with the begin and end function (which is an indirection to the begin and end function of the nodes filed).

LGTM overall.

And it got a lot more pleasant to read, especially the parts that used single letter variable names :-)

SimonDold · 2025-02-11T13:20:49Z

src/search/landmarks/landmark_graph.h

+    using const_iterator =
+        std::vector<std::unique_ptr<LandmarkNode>>::const_iterator;
+    const const_iterator begin() const {
+        return nodes.cbegin();


On use for cbegin is to communicate that a functor receiving the iterator does not change it.
With the function name LandmarkGraph::begin() this is not communicated anymore.

You are not using it this way and
LandmarkGraph::begin() is more readable than LandmarkGraph::cbegin() though.
So I am fine with it.

If I understood Remo correctly, naming the iterator cbegin didn't work with the range-based for loops as intended, but naming it begin did. Do you think it would make sense to write a comment about this or is it fine as is?

I think it is fine as is.

SimonDold · 2025-02-11T13:31:26Z

src/search/cartesian_abstractions/utils_landmarks.cc

-    for (const auto &parent_and_edge : node->parents) {
-        const LandmarkNode *parent = parent_and_edge.first;
+    open.reserve(node->parents.size());
+    for (const auto &[parent, type] : node->parents) {


Suggested change

for (const auto &[parent, type] : node->parents) {

for (const auto &[parent, _] : node->parents) {

Do you find the suggested option more readable? I know doing this is common practice in languages like Python because the symbol _ is actually interpreted differently than a variable name. However, in C++ both type and _ are (as of today) valid variable names and for the compiler it doesn't make a difference which one is used.

I think it communicates "we don't care about this part of the tuple".
I agree that it is a very pythonic suggestion and I am also fine to keep it as is.

SimonDold · 2025-02-11T13:36:42Z

src/search/cartesian_abstractions/utils_landmarks.cc

-            const LandmarkNode *parent = parent_and_edge.first;
+        FactPair ancestor_atom = get_atom(ancestor->get_landmark());
+        groups[ancestor_atom.var].push_back(ancestor_atom.value);
+        for (const auto &[parent, type] : ancestor->parents) {


SimonDold · 2025-02-11T13:41:42Z

src/search/landmarks/exploration.cc

-    }
-    unary_operators.reserve(num_unary_ops);
+    unary_operators.reserve(
+        compute_number_of_unary_operators(operators, axioms));

    // Build unary operators for operators and axioms.


This comment should be removed.

SimonDold · 2025-02-11T14:31:35Z

src/search/landmarks/exploration.cc

-                                   [precondition_fact.value]);
-
+        vector<Proposition *> precondition_propositions =
+            get_sorted_precondition_propositions(preconditions, effect);


This seems a bit wasteful to sort the combination of preconditions+effectcondition for each effect.
Instead one could sort the preconditions outside the for loop and then sort the effectconditions for each effect and combine the two like mergesort does.

SimonDold · 2025-02-11T16:24:18Z

src/search/landmarks/landmark_factory_rpg_sasp.cc

-                const FactPair pre_fact(pre.first, pre.second);
-                if (!lm_graph->contains_simple_landmark(pre_fact)) {
-                    preconditions[disj_class].push_back(pre_fact);
+                const FactPair atom(pre.first, pre.second);


Suggested change

const FactPair atom(pre.first, pre.second);

const FactPair precondition(pre.first, pre.second);

SimonDold · 2025-02-11T16:34:53Z

src/search/landmarks/landmark_graph.cc

+}
+
+LandmarkNode &LandmarkGraph::add_landmark(Landmark &&landmark_to_add) {
+    assert(landmark_to_add.is_conjunctive || all_of(


Suggested change

assert(landmark_to_add.is_conjunctive || all_of(

assert(landmark_to_add.is_conjunctive ||

all_of(landmark_to_add.atoms.begin(), landmark_to_add.atoms.end(),

[&](const FactPair &atom) {return !contains_landmark(atom);}));

SimonDold · 2025-02-11T16:37:16Z

src/search/landmarks/landmark_graph.cc

-        LandmarkNode &parent_node = *(parent.first);
-        parent_node.children.erase(node);
-        assert(parent_node.children.find(node) == parent_node.children.end());
+    for (const auto &[parent, type] : node->parents) {


Suggested change

for (const auto &[parent, type] : node->parents) {

for (const auto &[parent, _] : node->parents) {

SimonDold · 2025-02-11T16:37:39Z

src/search/landmarks/landmark_graph.cc

-        LandmarkNode &child_node = *(child.first);
-        child_node.parents.erase(node);
-        assert(child_node.parents.find(node) == child_node.parents.end());
+    for (const auto &[child, type] : node->children) {


Suggested change

for (const auto &[child, type] : node->children) {

for (const auto &[child, _] : node->children) {

SimonDold · 2025-02-11T16:44:21Z

src/search/landmarks/landmark_graph.cc

-                          return n.get() == node;
-                      });
+    const auto it =
+        find_if(nodes.begin(), nodes.end(),


In the header you introduce cbegin and cend.
Should this be used here too?

Clean up exploration class.

821f8c1

remochristen force-pushed the issue992 branch 3 times, most recently from a088dc1 to 4982fc3 Compare February 7, 2025 14:47

Make landmark graph iterable and rename landmark members.

c93adef

remochristen force-pushed the issue992 branch from 4982fc3 to c93adef Compare February 7, 2025 15:05

remochristen and others added 3 commits February 7, 2025 17:01

Rename edge to ordering.

7e47631

Rename functions and variables.

bf8198b

Clean up landmark_graph.cc and use HashSet instead of set.

7297587

ClemensBuechner force-pushed the issue992 branch 2 times, most recently from d0f3880 to a5ea42f Compare February 11, 2025 13:32

Rename most occurrences of lm in variable and function names.

c724ad5

ClemensBuechner force-pushed the issue992 branch from a5ea42f to c724ad5 Compare February 11, 2025 14:05

SimonDold reviewed Feb 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issue992 #246

issue992 #246

ClemensBuechner commented Feb 4, 2025

SimonDold left a comment

SimonDold Feb 11, 2025 •

edited

Loading

ClemensBuechner Feb 12, 2025

SimonDold Feb 12, 2025

SimonDold Feb 11, 2025

ClemensBuechner Feb 12, 2025

SimonDold Feb 12, 2025

SimonDold Feb 11, 2025

SimonDold Feb 11, 2025

SimonDold Feb 11, 2025

SimonDold Feb 11, 2025

SimonDold Feb 11, 2025

SimonDold Feb 11, 2025

SimonDold Feb 11, 2025

SimonDold Feb 11, 2025

	for (const auto &[parent, type] : node->parents) {
	for (const auto &[parent, _] : node->parents) {

	const FactPair atom(pre.first, pre.second);
	const FactPair precondition(pre.first, pre.second);

	for (const auto &[child, type] : node->children) {
	for (const auto &[child, _] : node->children) {

issue992 #246

Are you sure you want to change the base?

issue992 #246

Conversation

ClemensBuechner commented Feb 4, 2025

SimonDold left a comment

Choose a reason for hiding this comment

SimonDold Feb 11, 2025 • edited Loading

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

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

SimonDold Feb 11, 2025 •

edited

Loading