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.

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

First Order Logic #7608

Open
wants to merge 20 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@sdbiswas
Copy link

commented Jun 18, 2014

  • Make sure tests pass
  • Use Symbol everywhere, not strings (or Constant or Variable or whatever)
  • Add docstring with doctest to every class and function (including and especially abstract classes, although the abstract classes do not need to have doctests if that does not make sense). Major methods of classes should have this as well.
  • Address #7608 (comment)
  • Address #7608 (comment)
  • Use a single to_cnf and to_dnf and use dispatching to distinguish the two.
  • Change name for Functions.
  • Improve FOL_KB to handle multiple recursive clauses.
  • Implement Answer Extraction with the new ask.
  • Add description to Exceptions
  • Fix error in rst doc and add KB to it
self._func = func
self._args = tuple(args)

def _sympystr(self, *args, **kwargs):

This comment has been minimized.

Copy link
@asmeurer

asmeurer Jun 18, 2014

Member

The second argument should be the printer, I think. You should use it to recursively print the arguments.

This comment has been minimized.

Copy link
@sdbiswas

sdbiswas Jun 18, 2014

Author

All the printers here are purely for my personal convenience at this moment. All of the printer code will go into the respective file soon.

@asmeurer

This comment has been minimized.

Copy link
Member

commented Jun 18, 2014

I think you'll need to override _hashable_content along with __eq__. Python 3 make also make you override __hash__, which you should just pass to the superclass using super.

>>> from sympy.logic.FOL import Predicate
>>> Knows = Predicate('Knows')
>>> Knows('John', 'Jack')
Knows(John, Jack)

This comment has been minimized.

Copy link
@asmeurer

asmeurer Jun 18, 2014

Member

Are 'John' and 'Jack' implicitly converted to Symbol('John') and Symbol('Jack')?

This comment has been minimized.

Copy link
@sdbiswas

sdbiswas Jun 18, 2014

Author

No. The are intended to be constants.

This comment has been minimized.

Copy link
@asmeurer

asmeurer Jul 16, 2014

Member

The problem is that SymPy objects don't really play with with non-SymPy objects such as strings as part of their expression tree. In this case, I would just only use Symbol everywhere, since that is the standard way to have a 'name' in SymPy.

pass


class Function(Callable):

This comment has been minimized.

Copy link
@asmeurer

asmeurer Jun 18, 2014

Member

Is there a reason we can't use sympy.core.function.Function?

This comment has been minimized.

Copy link
@sdbiswas

sdbiswas Jun 18, 2014

Author

Except for the properties defined / overridden here, I don't think there is any other reason.

def __init__(self, var, expr):
try:
var = set(var)
except TypeError:

This comment has been minimized.

Copy link
@asmeurer

asmeurer Jun 18, 2014

Member

I think there's an iterable function in the compatibility module that would be better to use here, so that we remain standard across SymPy.

@asmeurer

This comment has been minimized.

Copy link
Member

commented Jun 18, 2014

I think it would be cleaner to just use symbols in place of strings. Most actual usage will be with symbols, I think. It's just a matter of using symbols in the doctests.

self._name = name

def __call__(self, *args):
return self.apply()(self, *args)

This comment has been minimized.

Copy link
@asmeurer

asmeurer Jul 16, 2014

Member

There should be some apply on this class, even if it just raises NotImplementedError.

@asmeurer

This comment has been minimized.

Copy link
Member

commented Jul 16, 2014

The class structure here is still a little confusing to me. Can you add a docstring to each class that just explains what it is for, especially the abstract base classes. It also should be made clear for each class if it is designed to be used on its own or if it is only a designed to be subclassed.

@asmeurer

This comment has been minimized.

Copy link
Member

commented Jul 22, 2014

I don't think it's a good idea to have multiple to_cnf functions. I would just have the one function, and use dispatching to have it do the right thing on different kinds of objects (i.e., have to_cnf call _eval_to_cnf on the object if it has that method, and then implement that on the logic and first order logic classes).

return expr.func(*args)


def standardize(expr):

This comment has been minimized.

Copy link
@asmeurer

asmeurer Jul 22, 2014

Member

You should probably add a keyword argument to pass in a custom iterator of symbols.

This comment has been minimized.

Copy link
@asmeurer

asmeurer Aug 8, 2014

Member

Similar to the symbols argument to cse().

def __call__(self, *args):
return self.apply()(self, *args)

def __eq__(self, other):

This comment has been minimized.

Copy link
@srjoglekar246

srjoglekar246 Aug 5, 2014

Member

Maybe you could override new instead of init, and pass Symbol(name) to the super-class constructor.
Something like
obj = super(Callable, cls).new(cls, Symbol(name))
obj._name = name
return obj

This way, you won't have to overwrite eq and hash. The SymPy core will take care of it automatically, by comparing the cls and the args.
Or maybe I missing something?

return self._name


class Applied(FOL):

This comment has been minimized.

Copy link
@srjoglekar246

srjoglekar246 Aug 5, 2014

Member

Maybe a small docstring here?

>>> Knows = Predicate('Knows')
>>> ForAll(X, ForAll(Y, Knows(X, Y)))
ForAll((X, Y), Knows(X, Y))

This comment has been minimized.

Copy link
@srjoglekar246

srjoglekar246 Aug 5, 2014

Member

Aah. I like the API for this one.

"""
Base class for all First Order Logic
"""
is_Fol = True

This comment has been minimized.

Copy link
@srjoglekar246

srjoglekar246 Aug 5, 2014

Member

If you are doing this, you will have to add is_Fol = False to Basic.
Why not FOL instead of Fol?

This comment has been minimized.

Copy link
@asmeurer

asmeurer Aug 8, 2014

Member

I would personally just not do this. It's better to use isinstance to tell what class it is.

if isinstance(expr, Quantifier):
return expr.func(expr.vars, _to_pnf(expr.expr))

raise ValueError()

This comment has been minimized.

Copy link
@srjoglekar246

srjoglekar246 Aug 5, 2014

Member

Maybe a small error message to tell a user what went wrong? Debugging can be a pain at times, if you don't know what exactly went wrong.

@asmeurer

This comment has been minimized.

Copy link
Member

commented Aug 8, 2014

I am going to put a checklist at the top of the pull request, so that we can keep track of what needs to be done.

raise ValueError("Use a constant instead of %s()" % func)
args = [arg if isinstance(arg, (Boolean, Symbol))
else Constant(arg) for arg in args]
self._func = func

This comment has been minimized.

Copy link
@asmeurer

asmeurer Aug 8, 2014

Member

Is this at all related to self.func (which will be the class, Applied)?

This comment has been minimized.

Copy link
@asmeurer

asmeurer Aug 18, 2014

Member

The issue with using a property is that it leads to confusion to people reading the code from here, as you can see from this comment of mine.

>>> T_domain = [1, 2, 3]
>>> person = lambda X: X in X_domain
>>> time = lambda T: T in T_domain
>>> CanFoolMap = {('John',2):False, ('John',3):False, 'default':True}

This comment has been minimized.

Copy link
@asmeurer

asmeurer Aug 8, 2014

Member

Put spaces after , and :.


def to_pnf(expr):
"""
Converts the given FOL expression into Prenex Normal Form.

This comment has been minimized.

Copy link
@asmeurer

asmeurer Aug 8, 2014

Member

What is Prenex Normal Form? (write it in the docstring)

"""
Converts the given FOL expression into Skolem Normal Form.
The formula in SNF is only equisatisfiable to the original
formula and not necessarily equivalent.

This comment has been minimized.

Copy link
@asmeurer

asmeurer Aug 8, 2014

Member

Explain what Skolem Normal Form is, and what the difference between equisatisfiable and equivalent is.

raise ValueError()


def to_snf(expr):

This comment has been minimized.

Copy link
@asmeurer

asmeurer Aug 8, 2014

Member

This should also optionally take generators as keyword arguments to customize the generation of numbered functions and constants.

return to_dnf_prop(expr)


def mgu(expr1, expr2):

This comment has been minimized.

Copy link
@asmeurer

asmeurer Aug 8, 2014

Member

We should think of a better name for this. Unless this is common elsewhere.


def mgu(expr1, expr2):
"""
Returns the Most General Unifier of two Predicates if it exists.

This comment has been minimized.

Copy link
@asmeurer

asmeurer Aug 8, 2014

Member

Since this is the function that Constant class is so important for, it might be worth it to explain how it will behave in this function (compared to the other classes).

I also don't see Constant mentioned in the code anywhere, so how is it actually handled differently? Is it just because it's not a subclass of Applied?

This comment has been minimized.

Copy link
@asmeurer

asmeurer Aug 8, 2014

Member

Can we reuse the unifier from the unify module here?


def resolve(*expr):
"""
Returns the resolution of given set of FOL formulas.

This comment has been minimized.

Copy link
@asmeurer

asmeurer Aug 8, 2014

Member

Explain what this means.

P(a, b),
P(b, c)
]
assert entails(P(a, c), formula_set) is True

This comment has been minimized.

Copy link
@asmeurer

asmeurer Aug 8, 2014

Member

Add more tests here, esp. some tests for False.

@asmeurer

This comment has been minimized.

Copy link
Member

commented Aug 12, 2014

I was hoping for more than just "abstract base class" in the docstrings of the abstract base classes. For someone coming to this module for the first time, it can be quite unclear what all the abstract classes are supposed to represent. It would be helpful to give a higher level idea of why they are there.

@asmeurer

This comment has been minimized.

Copy link
Member

commented Aug 12, 2014

I think the Python 3 test failures are coming from using strings instead of SymPy objects in the doctests.

@sdbiswas

This comment has been minimized.

Copy link
Author

commented Aug 12, 2014

  1. I'll work on better documentation.
  2. Simply calling to_snf followed by to_cnf/to_dnf (in boolalg) should give the user the required CNF/ DNF. to_snf returns an expression with no quantifiers (existentially bound variables are replaced by skolem functions and all other variables are considered to be universally quantified). Following this, any boolalg function can be called on this expression. The major requirement for CNF is for resolution, and the resolution method takes care of all this.
  3. I will fix it in some time.
@asmeurer

This comment has been minimized.

Copy link
Member

commented Aug 12, 2014

Simply calling to_snf followed by to_cnf/to_dnf (in boolalg) should give the user the required CNF/ DNF. to_snf returns an expression with no quantifiers (existentially bound variables are replaced by skolem functions and all other variables are considered to be universally quantified). Following this, any boolalg function can be called on this expression. The major requirement for CNF is for resolution, and the resolution method takes care of all this.

Good. I like this better. CNF and DNF should be only about propositional logic, and should treat quantifiers basic terms.

Soumya Dipta Biswas added some commits Aug 21, 2014

Soumya Dipta Biswas
Soumya Dipta Biswas
@asmeurer

This comment has been minimized.

Copy link
Member

commented Aug 22, 2014

As noted on gitter, the classes Predicate, AppliedPredicate, and Function need to be removed or renamed. I suggested reusing Predicate and AppliedPredicate from the assumptions, and renaming Function to BooleanUndefinedFunction

@asmeurer asmeurer added this to the 0.7.6 milestone Aug 27, 2014

@sdbiswas

This comment has been minimized.

Copy link
Author

commented Sep 9, 2014

@asmeurer I just want a reconfirmation of some of the things discussed before on gitter before I send in the (potentially) final commit for this;

  1. Regarding reusing Predicate, AppliedPredicate from assumptions:
    Would the user import Predicate from the assumptions then?
>>> from sympy.logic.FOL import Function, Constant, ForAll
>>> from sympy.assumptions import Predicate # Seem non-intuitive that user has to import something from assumptions to be able to use the FOL module

How about the renaming the class to BooleanUndefinedPredicate or FOL_Predicate (and corresponding Applied object), then subclassing assumptions predicate from this and adding only the extra methods there?

  1. Regarding BooleanUndefinedFunction:
    We had some discussion that the user would actually be instantiating BooleanFunction which will return a BooleanUndefinedFunction object. From a logical division perspective it doesn't seem like a good idea to me to mix boolalg code with FOL code. We should probably consider simply renaming the class (to FOL_Function probably) and leaving it as such.
@asmeurer

This comment has been minimized.

Copy link
Member

commented Sep 9, 2014

Seem non-intuitive that user has to import something from assumptions to be able to use the FOL module

You can import it again in the logic module, and you'll be able to import it there. Python imports any name from a module (except for builtins), regardless of whether they were actually defined there or just defined by being imported.

How about the renaming the class to BooleanUndefinedPredicate or FOL_Predicate (and corresponding Applied object), then subclassing assumptions predicate from this and adding only the extra methods there?

As far as naming goes, I'd say Predicate should be the generic version, and the subclass should have a different name (like AssumptionsPredicate or something).

From a logical division perspective it doesn't seem like a good idea to me to mix boolalg code with FOL code.

Can you expand on this?

@sdbiswas

This comment has been minimized.

Copy link
Author

commented Sep 9, 2014

  1. Oh, that should fix the problem then. The Predicate in FOL represents the most generic predicate possible. So subclassing other things from it makes sense. If the name of Predicate (in assumptions) is changed to AssumptionsPredicate I'll have to make changes throughout assumptions to reflect the same. A quick grep shows that currently assumptions, core/tests and solvers/inequalities use this name. Do you know of anything that needs to be changed?
  2. I think we should not make boolalg (BooleanFunction) dependent on FOL, simply from the perspective of logical separation. As far as possible we should keep the code for FOL limited to its file.
@asmeurer

This comment has been minimized.

Copy link
Member

commented Sep 9, 2014

  1. It shouldn't be used in many places. Also, if someone has code that uses Predicate and they don't update it to use AssumptionsPredicate the worst that will happen is that it won't print with the Q any more, right?
  2. But is BooleanUndefinedFunction useful outside of FOL?
@sdbiswas

This comment has been minimized.

Copy link
Author

commented Sep 9, 2014

  1. The AssumptionsPredicate has these extra methods: add_handler, remove_handler, sort_key, eval.
  2. BooleanUndefinedFunction has no use outside FOL
@asmeurer

This comment has been minimized.

Copy link
Member

commented Sep 9, 2014

add_handler and remove_handler are assumptions specific. The sort_key should stay on the Predicate object. I don't know what eval is, but it sounds like something that should stay on Predicate as well.

Remind me again what a BooleanUndefinedFunction represents. It's just a function f(X) except unlike Function('f') from the core, it is assumed to take on boolean values, right?

@asmeurer

This comment has been minimized.

Copy link
Member

commented Sep 13, 2014

@SDB1323 will you be able to finish this in the next week? Otherwise, it will need to be pushed to the next release.

@sdbiswas

This comment has been minimized.

Copy link
Author

commented Sep 13, 2014

Yes, it should be ready by the next weekend.

@sdbiswas

This comment has been minimized.

Copy link
Author

commented Sep 19, 2014

@asmeurer Please have a look at sdbiswas#1. Please confirm that this is what you want to be done finally. Additionally I need a little help from you because something is screwing up in ask once I do this.

@asmeurer

This comment has been minimized.

Copy link
Member

commented Sep 21, 2014

I'm removing the milestone. If you finish this, please let me know and we can still get it in, but I don't want to block the release on it.

@asmeurer asmeurer removed this from the 0.7.6 milestone Sep 21, 2014

@sdbiswas

This comment has been minimized.

Copy link
Author

commented Sep 21, 2014

Okay, sure. I am on this at the moment. I have modified the FOL classes to mimic the original Assumption classes. But that is creating errors in my code, so fixing that at the moment.

@peterwittek

This comment has been minimized.

Copy link

commented Jan 26, 2017

Is there any progress with this? Thanks.

@ghost

This comment has been minimized.

Copy link

commented Jan 19, 2018

I'm also interested in this, it appears to be halted now. Is there currently any plans for first-order logic in the future?

@sdbiswas

This comment has been minimized.

Copy link
Author

commented Jan 19, 2018

@naiveaiguy Please speak to the maintainers. I have been away for a very long time and don't know the current state of things here. If there is still interest in having FOL then I can help you with modifying and/or rewriting this code.

skirpichev added a commit to skirpichev/diofant that referenced this pull request Oct 29, 2018

First Order Logic
Mechanistic port of sympy/sympy#7608 for Diofant
by @skirpichev.

skirpichev added a commit to skirpichev/diofant that referenced this pull request Oct 29, 2018

First Order Logic
Mechanistic port of sympy/sympy#7608 for Diofant
by @skirpichev.

skirpichev added a commit to skirpichev/diofant that referenced this pull request Jan 13, 2019

First Order Logic
Mechanistic port of sympy/sympy#7608 for Diofant

// edited by @skirpichev

100% test coverage.

skirpichev added a commit to skirpichev/diofant that referenced this pull request Jan 13, 2019

First Order Logic
Mechanistic port of sympy/sympy#7608 for Diofant

// edited by @skirpichev

100% test coverage.

skirpichev added a commit to skirpichev/diofant that referenced this pull request Feb 28, 2019

First Order Logic
Mechanistic port of sympy/sympy#7608 for Diofant

// edited by @skirpichev

100% test coverage.

@ShubhamKJha ShubhamKJha referenced this pull request Jun 22, 2019

Open

[WIP]First Order Logic with Equality #17069

1 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.