aig-upf / tarski

Tarski - An AI Planning Modeling Framework
Apache License 2.0
59 stars 20 forks source link

Uniformize Predicate and Function interface #53

Closed gfrances closed 3 years ago

gfrances commented 5 years ago

The current design is not too consistent between classes Predicate and Function. They share no code, which is not nice (given that they are extremely similar, except for functions having a codomain, which for predicates is assumed to be boolean). Even worse, in one of them the main attribute is called symbol, whereas in the other it's _symbol, and then a @property is there to help make things uniform. This "symbol" is sometimes a string, sometimes an object of type BuiltinSymbol, etc.

In short, I would like to address this somewhat minor inconsistencies that however make other code in many places be unnecessarily complex, by forcing it to subcase for predicates and functions, etc.

gfrances commented 5 years ago

(Working on this already)

gfrances commented 5 years ago

@miquelramirez, I am commiting this to a hopefully short-lived branch dev-0.2.0-symbols, perhaps you could checkout if you find a few minutes and see if it breaks any code of yours? Tests pass correctly, but I would expect some things to be broken by attribute name changes, hence the new branch...

miquelramirez commented 5 years ago

Hi @gfrances,

it breaks everything, as the compilations do need to access the name of the function or predicate symbol to map it into the target language.

I see that you haven't transformed all terms yet (constants have a symbol not a name), and that there's now a new member called head defined for all CompoundTerms (but the subclasses haven't been changed).

I do think that the changes are important, but I would rather have just two classes: Function being the abstract one, and Predicate the specialization (as Predicates are an special case of functions, those mapping to Booleans).

I am cool with "breaking everything" as long as there is clear migration guide so changes can be applied in bulk.

Miquel.

gfrances commented 5 years ago

Hehe you're right, I only started doing some changes. The general rationale for changing symbol to name was to call "symbol" those attributes that are indeed a symbol (i.e. a function symbol or a predicate symbol); and call "name" those attributes that are just strings or literals. OTOH, the rationale for changing the interface of CompoundTerms was to have both CompoundTerm and Atom present the same interface (a head plus a number of subterms).

I wouldn't be opposed to "merge" Functions and Predicates. I think it's less clear, but then it simplifies a lot of code, and besides it seems to be a rather common convention in the code of some other popular libraries (z3, yices, etc.). That would however mean that we should probably merge the "Formula" and "Term" hierarchy - were you implying that as well, or only change Function and Predicate symbols classes?

miquelramirez commented 5 years ago

Hi @gfrances,

I think I like better merging Functions and Predicates as it simplifies very much the presentation of planning models based on FSTRIPS. I find very easy - read, parsimonious - to find succinct and compact notation to define concepts, and also simplifies proving soundness and efficiency of reductions (or lack thereof). In my view, predicate is the special case - an important special case - which has a long tradition in the literature in KR and logic as many and powerful theorem provers and solvers are specialised to deal with predicates, whose denotation is given by a extension (or intensionally in the case of Boolean CNF formulae for instance). With hindsight, I think it is weird that we think of functions as an extension to Boolean formulas (I mean, probably other communities find this weird too).

On the other hand, I think it is a good idea to keep the separation between formula and term, as the major distinction between both, besides restrictions on the codomain (Boolean or else) is in how we evaluate them, something which is completely internal (and requiring quite different strategies and concerns). Note that our interpretation operator

I[x(p) == 1]

evaluates relational formulae (atoms) like the one above to true or false. Hence, from the user point of view, there's no real difference between either, since the interface to evaluate them is uniform.

Having said that having an Expression superclass for both Formula and Term may be useful to avoid duplicating interfaces and functionality (I am thinking of the support for TermReference and FormulaReference for instance, maybe we only want ExpressionReference).

Regarding name and head I think both are good changes: symbol is the current smurf word for Tarski.

gfrances commented 5 years ago

Upon further reflection, I think we should split this into two different issues. I'll leave this one open here with the discussion on the potential merge of Predicate and Function classes. I'm not fully convinced that we should rush to do that - I see advantages and disadvantages, and we already have a working version, so not sure I would prioritize changing that. However, I've opened issue #56 to deal with the name consistency part of this issue - that's an easy one, and I think it's important.