get rid of g_axiom_evaluators #235
Conversation
| @@ -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(); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
I don't understand this class well enough to review this part of the pull request.
There was a problem hiding this comment.
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.
No description provided.