Skip to content

get rid of g_axiom_evaluators #235

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
FlorianPommerening wants to merge 1 commit into
base: main
Choose a base branch
from
Open

get rid of g_axiom_evaluators #235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/search/abstract_task.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
#include <utility>
#include <vector>

class AxiomEvaluator;

struct FactPair {
int var;
int value;
Expand Down Expand Up @@ -101,6 +103,8 @@ class AbstractTask : public subscriber::SubscriberService<AbstractTask> {
virtual void convert_ancestor_state_values(
std::vector<int> &values,
const AbstractTask *ancestor_task) const = 0;

virtual AxiomEvaluator &get_axiom_evaluator() const = 0;
};

#endif
2 changes: 0 additions & 2 deletions src/search/axioms.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,5 +151,3 @@ void AxiomEvaluator::evaluate(vector<int> &state) {
}
}
}

PerTaskInformation<AxiomEvaluator> g_axiom_evaluators;
5 changes: 0 additions & 5 deletions src/search/axioms.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
#ifndef AXIOMS_H
#define AXIOMS_H

#include "per_task_information.h"
#include "task_proxy.h"

#include <memory>
#include <vector>

class AxiomEvaluator {
Expand Down Expand Up @@ -61,7 +59,4 @@ class AxiomEvaluator {

void evaluate(std::vector<int> &state);
};

extern PerTaskInformation<AxiomEvaluator> g_axiom_evaluators;

#endif
2 changes: 1 addition & 1 deletion src/search/state_registry.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ using namespace std;
StateRegistry::StateRegistry(const TaskProxy &task_proxy)
: task_proxy(task_proxy),
state_packer(task_properties::g_state_packers[task_proxy]),
axiom_evaluator(g_axiom_evaluators[task_proxy]),
axiom_evaluator(task_proxy.get_axiom_evaluator()),
num_variables(task_proxy.get_variables().size()),
state_data_pool(get_bins_per_state()),
registered_states(
Expand Down
2 changes: 1 addition & 1 deletion src/search/task_proxy.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ State State::get_unregistered_successor(const OperatorProxy &op) const {
}

if (task->get_num_axioms() > 0) {
AxiomEvaluator &axiom_evaluator = g_axiom_evaluators[TaskProxy(*task)];
AxiomEvaluator &axiom_evaluator = TaskProxy(*task).get_axiom_evaluator();
axiom_evaluator.evaluate(new_values);
}
return State(*task, move(new_values));
Expand Down
4 changes: 4 additions & 0 deletions src/search/task_proxy.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include <vector>


class AxiomEvaluator;
class AxiomsProxy;
class ConditionsProxy;
class EffectProxy;
Expand Down Expand Up @@ -725,6 +726,9 @@ class TaskProxy {
}

const causal_graph::CausalGraph &get_causal_graph() const;
AxiomEvaluator &get_axiom_evaluator() const {
return task->get_axiom_evaluator();
}
};


Expand Down
8 changes: 6 additions & 2 deletions src/search/tasks/default_value_axioms_task.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ DefaultValueAxiomsTask::DefaultValueAxiomsTask(
FactPair(var, default_value), axiom_ids);
}
}
axiom_evaluator = make_unique<AxiomEvaluator>(TaskProxy(*this));
}

/*
Expand Down Expand Up @@ -388,13 +389,16 @@ int DefaultValueAxiomsTask::get_num_axioms() const {
return parent->get_num_axioms() + default_value_axioms.size();
}

AxiomEvaluator &DefaultValueAxiomsTask::get_axiom_evaluator() const {
return *axiom_evaluator;
}

shared_ptr<AbstractTask> get_default_value_axioms_task_if_needed(
const shared_ptr<AbstractTask> &task,
AxiomHandlingType axioms) {
TaskProxy proxy(*task);
if (task_properties::has_axioms(proxy)) {
return make_shared<tasks::DefaultValueAxiomsTask>(
DefaultValueAxiomsTask(task, axioms));
return make_shared<tasks::DefaultValueAxiomsTask>(task, axioms);
Copy link
Copy Markdown
Contributor

@maltehelmert maltehelmert Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this class well enough to review this part of the pull request.

Copy link
Copy Markdown
Member Author

@FlorianPommerening FlorianPommerening Dec 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This particular change looks harmless to me: it's just getting rid of an unnecessary copy.

Other than that, the current change is to not delegate the axiom evaluator here and instead create one from scratch. From what I understood from the discussion with Gabi about this, this is a mistake: Tasks of this kind have no well-defined semantics for the axiom evaluation and the new axioms are only meant to be used by the heuristic not to generate states. In the old code, this is only enforced with documentation, with the get_axiom_evaluator method, we could throw an error if someone tries to access an axiom evaluator for a task like this.

}
return task;
}
Expand Down
3 changes: 3 additions & 0 deletions src/search/tasks/default_value_axioms_task.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

#include "delegating_task.h"

#include "../axioms.h"
#include "../plugins/plugin.h"

#include <set>
Expand Down Expand Up @@ -49,6 +50,7 @@ class DefaultValueAxiomsTask : public DelegatingTask {
AxiomHandlingType axioms;
std::vector<DefaultValueAxiom> default_value_axioms;
int default_value_axioms_start_index;
std::unique_ptr<AxiomEvaluator> axiom_evaluator;

std::unordered_set<int> get_vars_with_relevant_default_value(
const std::vector<std::vector<int>> &nondefault_dependencies,
Expand Down Expand Up @@ -81,6 +83,7 @@ class DefaultValueAxiomsTask : public DelegatingTask {
int op_index, int eff_index, bool is_axiom) const override;

virtual int get_num_axioms() const override;
virtual AxiomEvaluator &get_axiom_evaluator() const override;
};

extern std::shared_ptr<AbstractTask> get_default_value_axioms_task_if_needed(
Expand Down
4 changes: 4 additions & 0 deletions src/search/tasks/delegating_task.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,4 +108,8 @@ void DelegatingTask::convert_ancestor_state_values(
parent->convert_ancestor_state_values(values, ancestor_task);
convert_state_values_from_parent(values);
}

AxiomEvaluator &DelegatingTask::get_axiom_evaluator() const {
return parent->get_axiom_evaluator();
Copy link
Copy Markdown
Contributor

@maltehelmert maltehelmert Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we need to discuss this design more. There is a difference between the other delegation methods and this one in the sense that the other ones delegate the default but can override the behavior because we want to be able to change the behavior. In contrast, with axioms evaluators there is no conceptual reason to override the behavior; the axiom evaluator should always correctly implement the axioms of the tasks, which are defined by the other methods.

So we're now adding a new responsibility to the tasks to make sure that this happens (rather than having the methods that return the axioms and the method that returns the axiom evaluator behave inconsistently).

I understand that there can be a reason here to delegate for efficiency reasons, but I think correctness would be a better default than efficiency. And I think it needs to be clearer (from the documentation or whatever) in which scenarios one would/would not override this method and why.

Copy link
Copy Markdown
Member Author

@FlorianPommerening FlorianPommerening Dec 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this similar to other methods in the abstract task interface? For example get_num_operators and get_operator_name have to be consistent with each other, i.e., if a delegating task changes the number of operators, it has to override both methods and all others related to operators. In that sense, I think it is consistent that if a delegating task changes something about the axioms, it has to override all methods relating to axioms, including get_axiom_evaluator. But I get the point that it would be better if the task wouldn't be responsible for consistency between the returned axioms (at the task proxy level) and the returned axiom evaluator.

I'm happy to discuss the design. Perhaps we can find a solution where delegation is possible but you have to be explicit about re-using an evaluator? Just creating the evaluator in the base class of delegating tasks would achieve that but that is a bit inconsistent with the other methods of it: so far, if you don't change anything in a delegating task, you just get the parent task.

}
}
2 changes: 2 additions & 0 deletions src/search/tasks/delegating_task.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ class DelegatingTask : public AbstractTask {
const AbstractTask *ancestor_task) const final override;
virtual void convert_state_values_from_parent(std::vector<int> &) const {
}

virtual AxiomEvaluator &get_axiom_evaluator() const override;
};
}

Expand Down
12 changes: 9 additions & 3 deletions src/search/tasks/root_task.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ class RootTask : public AbstractTask {
vector<vector<set<FactPair>>> mutexes;
vector<ExplicitOperator> operators;
vector<ExplicitOperator> axioms;
unique_ptr<AxiomEvaluator> axiom_evaluator;
vector<int> initial_state_values;
vector<FactPair> goals;

Expand Down Expand Up @@ -105,6 +106,7 @@ class RootTask : public AbstractTask {
virtual void convert_ancestor_state_values(
vector<int> &values,
const AbstractTask *ancestor_task) const override;
virtual AxiomEvaluator &get_axiom_evaluator() const override;
};


Expand Down Expand Up @@ -351,11 +353,11 @@ RootTask::RootTask(istream &in) {
have reached the end of "in". */

/*
HACK: We use a TaskProxy to access g_axiom_evaluators here which assumes
HACK: We use a TaskProxy here which assumes
that this task is completely constructed.
*/
AxiomEvaluator &axiom_evaluator = g_axiom_evaluators[TaskProxy(*this)];
axiom_evaluator.evaluate(initial_state_values);
axiom_evaluator = make_unique<AxiomEvaluator>(TaskProxy(*this));
axiom_evaluator->evaluate(initial_state_values);
}

const ExplicitVariable &RootTask::get_variable(int var) const {
Expand Down Expand Up @@ -492,6 +494,10 @@ void RootTask::convert_ancestor_state_values(
}
}

AxiomEvaluator &RootTask::get_axiom_evaluator() const {
return *axiom_evaluator;
}

void read_root_task(istream &in) {
assert(!g_root_task);
g_root_task = make_shared<RootTask>(in);
Expand Down