Skip to content
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

get rid of g_axiom_evaluators #235

Open
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
Contributor

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.

}
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
Contributor

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.

}
}
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