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

add force to to_cnf and to_dnf; is_literal made more general #18912

Merged
merged 8 commits into from
Mar 21, 2020

Conversation

smichr
Copy link
Member

@smichr smichr commented Mar 20, 2020

References to other Issues or PRs

fixes #9949
fixes #18904

Brief description of what is fixed or changed

When to_cnf is given an expression with more than 8 variables and simplify=True it will not simplify the expression -- and may not return it in cnf form and does so silently. Now, a ValueError is raised if simplification is requested and force is not True. The user has the option to not use simplify or to use force.

In addition, is_literal and is_a_literal were combined and made to recognize that things like x < 2 can be treated like a literal. This fixes issue 9949.

Finally, a quick-exit is given to simplify_logic to deal with trivial cases that don't need simplification and are already in the correct, most implified form.

Other comments

Release Notes

  • logic
    • boolalg
      • to_cnf/to_dnf (when simplify=True) require force=True` if there are more than 8 variables
      • simplify_logic recognizes trivial simplified cases
      • is_literal can treat Not as literal or not by using the literal_Not flag

@sympy-bot
Copy link

sympy-bot commented Mar 20, 2020

Hi, I am the SymPy bot (v158). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

  • logic
    • boolalg (#18912 by @smichr)

    • to_cnf/to_dnf (when simplify=True) require force=True` if there are more than 8 variables (#18912 by @smichr)

    • simplify_logic recognizes trivial simplified cases (#18912 by @smichr)

    • is_literal can treat Not as literal or not by using the literal_Not flag (#18912 by @smichr)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.6.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->

fixes #9949
fixes #18904

#### Brief description of what is fixed or changed

When `to_cnf` is given an expression with more than 8 variables and `simplify=True` it will not simplify the expression -- and may not return it in cnf form and does so silently. Now, a ValueError is raised if simplification is requested and `force` is not True. The user has the option to not use `simplify` or to use `force`.

In addition, `is_literal` and `is_a_literal` were combined and made to recognize that things like `x < 2` can be treated like a literal. This fixes issue 9949.

Finally, a quick-exit is given to `simplify_logic` to deal with trivial cases that don't need simplification and are already in the correct, most implified form.

#### Other comments


#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* logic
    * boolalg
        * `to_cnf`/`to_dnf` (when `simplify=True) require `force=True` if there are more than 8 variables
        * `simplify_logic` recognizes trivial simplified cases
        * `is_literal` can treat `Not` as literal or not by using the `literal_Not` flag
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@codecov
Copy link

codecov bot commented Mar 20, 2020

Codecov Report

Merging #18912 into master will increase coverage by 0.031%.
The diff coverage is 90.476%.

@@              Coverage Diff              @@
##            master    #18912       +/-   ##
=============================================
+ Coverage   75.648%   75.679%   +0.031%     
=============================================
  Files          647       647               
  Lines       168520    168535       +15     
  Branches     39710     39713        +3     
=============================================
+ Hits        127483    127547       +64     
+ Misses       35476     35439       -37     
+ Partials      5561      5549       -12

@asmeurer
Copy link
Member

What is the motivation for the literal_Not flag? The inclusion of Not is based on a standard definition of "literal".

@smichr
Copy link
Member Author

smichr commented Mar 20, 2020

What is the motivation for the literal_Not flag

Hmm...I must have had a different problem when trying to detect the quick exit case for simplify_logic. When I retested now, everything passed. Let's see if the full suite raises any objections...

@@ -2782,6 +2793,16 @@ def simplify_logic(expr, form=None, deep=True, force=False):
if form not in (None, 'cnf', 'dnf'):
raise ValueError("form can be cnf or dnf only")
expr = sympify(expr)
# check for quick exit: right form and all args are
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewer: please check to see that this is the right logic for quick exit: if an expression is in the right form and all args are literal then there is nothing left to do.

return expr.args[0].is_Atom
elif expr in (True, False) or expr.is_Atom:
return True
elif not isinstance(expr, BooleanFunction) and all(
Copy link
Member Author

@smichr smichr Mar 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g. x < 3 is treated like a literal but x + y < 3 would not be a literal.

sympy/logic/boolalg.py Outdated Show resolved Hide resolved
sympy/logic/boolalg.py Outdated Show resolved Hide resolved
@smichr smichr merged commit acca306 into sympy:master Mar 21, 2020
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.

is_cnf(to_cnf(expr)) should be True boolalg.py:is_cnf fails when it should pass
3 participants