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

Fix KBOForEPR polymorphism bug #301

Merged
merged 7 commits into from
Nov 30, 2021
Merged

Conversation

ibnyusuf
Copy link
Member

@ibnyusuf ibnyusuf commented Nov 25, 2021

KBOForEPR assumes that all arguments to a predicate symbol are either variables or constants. We were not checking this properly for polymorphic predicate symbols previously. This PR should fix the bug.

Shell/Property.cpp Outdated Show resolved Hide resolved
@MichaelRawson
Copy link
Contributor

Nice! Theoretical question: if we have only fully-applied type constructors (like in the Z3 PR), could we still use KBOForEPR in that case?

@ibnyusuf
Copy link
Member Author

Nice! Theoretical question: if we have only fully-applied type constructors (like in the Z3 PR), could we still use KBOForEPR in that case?

No. In the current implementation of KBOforEPR, we make certain assumptions that are no longer valid in the presence of function symbols or type constructors with arity > 0.

For example:

Comparison arComp=Int::compare(l1->arity(),l2->arity());
    if(arComp!=Lib::EQUAL) {
      //since on the ground level each literal argument must be a constant,
      //and all symbols are of weight 1, the literal with higher arity is
      //heavier and therefore greater
      return fromComparison(arComp);
    }

As soon as we allow symbols of arity > 0 the weight of the ground instance of a variable can be arbitrarily large, and the above optimisation is no longer valid.

@quickbeam123 quickbeam123 merged commit 106b95b into master Nov 30, 2021
@quickbeam123 quickbeam123 deleted the ahmed-fix-kboforepr-bug branch November 30, 2021 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants