Jorge/get body callback

[jorgefandinno]

I realized that get_body() is not general enough for eclingo. For instance, with rule

a(X) :- &k{ b(X) }, &k{ not c(X)}.

we want to obtain the positive body

["&k { b(X) }"]

See test https://github.com/jorgefandinno/python-clingox/blob/1ca03c8e071192f3bf78995e70490afe8aa5e9b5/clingox/tests/test_ast.py#L2584

Obviously , this is eclingo specific. In general, the user of the library should be able to inspect the theory atom to decide if it should be excluded or not.

This pull request achieves that by allowing to pass a callback as exclude_theory_atoms

[rkaminsk]

Now the interface looks a bit random. What about simplifying the signature:

def get_body(
    stm: AST,
    predicate: Callable[[AST], bool] = None,
    signs: Container[Sign] = (Sign.NoSign, Sign.Negation, Sign.DoubleNegation),
) -> List[AST]:
    """
    Get body literals satisfying the (optional) predicate with the given signs.
    """

You use case could then be written as:

body = get_body(stm, lambda x: (
    x.ast_type == ASTType.Literal
    and x.atm.ast_type == ASTType.TheoryAtom
    and x.elements
    and x.elements[0].terms
    and x.elements[0].terms[0].ast_type == ASTType.TheoryUnparsedTerm
    and x.elements[0].terms[0].elements
    and x.elements[0].terms[0].elements[0].ast_type == ASTType.TheoryUnparsedTermElement
    and x.elements[0].terms[0].elements[0].operators
    and x.elements[0].terms[0].elements[0].operators[0] == "not"
)

Alternatively, we could also have a predicate for each type of body literal. Not sure what is the nicest solution.

PS: In Python it is customary to test if a sequence is empty using if seq: instead of if len(seq) > 0:.

[jorgefandinno]

Alternatively, we could also have a predicate for each type of body literal. Not sure what is the nicest solution.

I find it easier to use having a predicate for each type. Let me know, I can change it.

PS: In Python it is customary to test if a sequence is empty using if seq: instead of if len(seq) > 0:.

I know, and I really don't like it. It seems to be against the own Python principle of "explicit is better than implicit". Anything can be used in an if and good luck getting what it is without reading the whole code. If I read if seq:, seq my be a bool, a sequence of actually anything that I am checking for None. If I write if len(seq) > 0: I know that it must be a container (or a string).
Still, if you want to be used that way, I will. Just let me know.

[rkaminsk]

Alternatively, we could also have a predicate for each type of body literal. Not sure what is the nicest solution.

I find it easier to use having a predicate for each type. Let me know, I can change it.

Sure, it would be a bit like a customizable visitor like this. I just don't think we need a union of a bool/callable then. Just a callable is enough (which can be None btw.).

PS: In Python it is customary to test if a sequence is empty using if seq: instead of if len(seq) > 0:.

I know, and I really don't like it. It seems to be against the own Python principle of "explicit is better than implicit". Anything can be used in an if and good luck getting what it is without reading the whole code. If I read if seq:, seq my be a bool, a sequence of actually anything that I am checking for None. If I write if len(seq) > 0: I know that it must be a container (or a string). Still, if you want to be used that way, I will. Just let me know.

It's the recommended way. Check https://peps.python.org/pep-0008/#programming-recommendations. You'll get used to it.

[jorgefandinno]

Sure, it would be a bit like a customizable visitor like this. I just don't think we need a union of a bool/callable then. Just a callable is enough (which can be None btw.).

I change the signature mostly as we discussed.

def get_body(
    stm: AST,
    symbolic_atom_predicate: Union[Callable[[AST], bool], bool] = True,
    theory_atoms_predicate: Union[Callable[[AST], bool], bool] = True,
    aggregate_predicate: Union[Callable[[AST], bool], bool] = True,
    conditional_literals_predicate: Union[Callable[[AST], bool], bool] = True,
    signs: Container[Sign] = (Sign.NoSign, Sign.Negation, Sign.DoubleNegation),
) -> List[AST]:

It seems to me that Union[Callable[[AST], bool], bool] allows a more clear use than Optional[Callable[[AST], bool]] in the sense that we use True and False in place of the functions that always return these values (lambda x: True and lambda x: False). This is very common in ASP: the empty body is true and the empty head is false. Anyway, I am not religious about it and can be changed. The only drawback is that we will have to write lambda x: False instead of False when calling it.

We may create a type aliases

Predicate = Union[Callable[[AST], bool], bool]

There are still a couple of things that I think deserve improvement.

• The first is the the treatment of sings in conditional literals. Take for instance, test case

self.helper_get_body(
    "a(X) :- b(X), not c(Y), d(Z): e(X,Z); not d(Z): e(X,Z).",
    ["b(X)", "d(Z): e(X,Z)", "not d(Z): e(X,Z)"],
    signs=(Sign.NoSign,),
)

It seems to me that it would be natural that passing signs=(Sign.NoSign,) gives you the positive body, but "not d(Z): e(X,Z)" is not part of it. Currently this can be addressed by passing in addition the appropriate conditional_literals_predicate as follows

        self.helper_get_body(
            "a(X) :- b(X), not c(Y), d(Z): e(X,Z); not d(Z): e(X,Z).",
            ["b(X)", "d(Z): e(X,Z)"],
            signs=(Sign.NoSign,),
            conditional_literals_predicate=lambda x: x.literal.sign != Sign.Negation,
        )

but I think it would be more clear that it worked just with the former. Tought, slightly less configurable.

• The second is making it return a generator instead of a list. Alternatively, make it return a pair of lists, one with the one that satisfies the conditions and the other with the rest. Both will make slightly more efficient some use cases. Do you see any drawbacks on any of them? Some preference?

What do you think?

It's the recommended way. Check https://peps.python.org/pep-0008/#programming-recommendations. You'll get used to it.

Ok, ok... done, I will stop thinking by myself :) It is good to know that I am not alone on this (https://youtu.be/S0No2zSJmks?t=788), though.

[rkaminsk]

I change the signature mostly as we discussed.

def get_body(
    stm: AST,
    symbolic_atom_predicate: Union[Callable[[AST], bool], bool] = True,
    theory_atoms_predicate: Union[Callable[[AST], bool], bool] = True,
    aggregate_predicate: Union[Callable[[AST], bool], bool] = True,
    conditional_literals_predicate: Union[Callable[[AST], bool], bool] = True,
    signs: Container[Sign] = (Sign.NoSign, Sign.Negation, Sign.DoubleNegation),
) -> List[AST]:

It seems to me that Union[Callable[[AST], bool], bool] allows a more clear use than Optional[Callable[[AST], bool]] in the sense that we use True and False in place of the functions that always return these values (lambda x: True and lambda x: False). This is very common in ASP: the empty body is true and the empty head is false. Anyway, I am not religious about it and can be changed. The only drawback is that we will have to write lambda x: False instead of False when calling it.

We can do it as you suggest. Please decide whether you want to use singular or plural though. Maybe also use callable(predicate) instead of isinstance(predicate, bool). It should not matter with properly typed programs but there might be people who want to pass in bool-like objects.

We may create a type aliases

Predicate = Union[Callable[[AST], bool], bool]

Sure, but maybe call it ASTPredicate.

There are still a couple of things that I think deserve improvement.

* The first is the the treatment of sings in conditional literals. Take for instance, test case

self.helper_get_body(
    "a(X) :- b(X), not c(Y), d(Z): e(X,Z); not d(Z): e(X,Z).",
    ["b(X)", "d(Z): e(X,Z)", "not d(Z): e(X,Z)"],
    signs=(Sign.NoSign,),
)

It seems to me that it would be natural that passing signs=(Sign.NoSign,) gives you the positive body, but "not d(Z): e(X,Z)" is not part of it. Currently this can be addressed by passing in addition the appropriate conditional_literals_predicate as follows

        self.helper_get_body(
            "a(X) :- b(X), not c(Y), d(Z): e(X,Z); not d(Z): e(X,Z).",
            ["b(X)", "d(Z): e(X,Z)"],
            signs=(Sign.NoSign,),
            conditional_literals_predicate=lambda x: x.literal.sign != Sign.Negation,
        )

but I think it would be more clear that it worked just with the former. Tought, slightly less configurable.

Yes, it is probably a good idea consider the sign of the head of a conditional literal, too.

* The second is making it return a generator instead of a list. Alternatively, make it return a pair of lists, one with the one that satisfies the conditions and the other with the rest. Both will make slightly more efficient some use cases. Do you see any drawbacks on any of them? Some preference?

Returning two lists would rather be a partition_body function. I don't think we have to care too much about efficiency here. In practice, non-ground logic programs should be small. One has to take more care with ground representations. We could make the function a generator. I don't think that efficiency is an issue but a generator can easily be converted into a list or any other sequence type if needed. Note that the generator approach won't work nicely with partitioning. You could add a complement flag that inverts the meaning of the predicates if you think that this would be useful.

Ok, ok... done, I will stop thinking by myself :) It is good to know that I am not alone on this (https://youtu.be/S0No2zSJmks?t=788), though.

Interesting that he compares with C/C++. Modern guidelines say one should write != 0 or != nullptr. So in C++ I do it differently than in Python. It's best not to think too much about this. 😄

[jorgefandinno]

Done

You could add a complement flag that inverts the meaning of the predicates if you think that this would be useful.

I don't think so, I hope people can write a not by themselves.

Ok, ok... done, I will stop thinking by myself :) It is good to know that I am not alone on this (https://youtu.be/S0No2zSJmks?t=788), though.

Interesting that he compares with C/C++. Modern guidelines say one should write != 0 or != nullptr. So in C++ I do it differently than in Python. It's best not to think too much about this. 😄

Yeah, specially given that in C++ 0-overhead is a priority. Not in pyhton.