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

logic equation suggestions #16194

Merged
merged 5 commits into from
Jun 25, 2019
Merged

logic equation suggestions #16194

merged 5 commits into from
Jun 25, 2019

Conversation

shubhamabhang77
Copy link
Contributor

@shubhamabhang77 shubhamabhang77 commented Mar 7, 2019

References to other Issues or PRs

#16137

Brief description of what is fixed or changed

expr._rewrite_as_And, expr._rewrite_as_Or, expr._rewrite_as_Nor, expr._rewrite_as_Nand
added to logic equations.

Other comments

Release Notes

  • logic
    • And can be rewritten as Nor, Or as Nand, and Xor as either Oror And.

@sympy-bot
Copy link

sympy-bot commented Mar 7, 2019

Hi, I am the SymPy bot (v147). 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:

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

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://github.com/blog/1506-closing-issues-via-pull-requests . Please also
write a comment on that issue linking back to this pull request once it is
open. -->
#16137 

#### Brief description of what is fixed or changed
```expr._rewrite_as_And, expr._rewrite_as_Or, expr._rewrite_as_Nor, expr._rewrite_as_Nand```
added to logic equations.

#### 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
    * `And` can be rewritten as `Nor`, `Or` as `Nand`, and `Xor` as either `Or`or `And`.

<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

sympy/logic/boolalg.py Outdated Show resolved Hide resolved
@Abdullahjavednesar Abdullahjavednesar added logic PR: author's turn The PR has been reviewed and the author needs to submit more changes. labels Mar 7, 2019
sympy/logic/boolalg.py Outdated Show resolved Hide resolved
sympy/logic/boolalg.py Outdated Show resolved Hide resolved
return Or(And(Not(arguments[0]), arguments[1]), And(Not(arguments[1]), arguments[0]))
else:
for i in range(len(arguments)-1):
eq = Or(And(Not(arguments[i]), arguments[i+1]), And(Not(arguments[i+1]), arguments[i])).simplify()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is Xor defined for more than two variables? In that case, how? (I would assume parity, although it can be argued if there actually is an XOR-function for more than two variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I searched regarding XOR of more than 2 inputs then it was showing that it exists. So I did this. Without simplify() output was not in simplified form. We can do one thing rather using simplify multiple times we can use it while returning.

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked the documentation and Xor seems to be parity/add-modulo-2 so OK with multiple inputs (although the doubt was more from a formal perspective, it makes senses from a practical perspective).

OK. I guess that one can figure out a way to write it by looking at e.g. SOPform and the line
Or(*[_convert_to_varsSOP(x, variables) for x in essential]) where essential would be a list of all combinations of len(args) variables with an odd number of ones (can be generated by ibin, see sympy.utilities.iterables, and some filtering) and variables are args.

Not sure if it is required though. But feel free to give it a try. For a three-input Xor, the list essential should look like [[1, 0, 0], [0, 1, 0], [0, 0, 1], [1, 1, 1]] and so on.

Copy link
Member

Choose a reason for hiding this comment

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

Can we reuse one of the other methods here, like to_cnf or to_dnf?

Copy link
Contributor

Choose a reason for hiding this comment

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

It does not give the same result if the arguments are boolean functions in themselves.

to_cnf(Xor(And(x, z), y, z))
image

Xor(And(x, z), y, z).rewrite(And)
image

(With reservations for that I have messed up my installation running two different sources...)

They both simplify to the same expression though. It think the rewrite approach is slightly more suitable here as one most likely would except the expression on a certain form (not cnf, but as a rewrite based on the original expression). (I'm also a bit surprised that the z and ~z is simplified in the last cnf-expression.)

Of course one can argue which is the "best" form... One way is of course to just recreate a minimal cnf/nnf (SOP/POS) expression, but since there are other methods for that, I think this makes sense.

@oscargus
Copy link
Contributor

oscargus commented Mar 9, 2019

Ideally it should be possible to call expr.rewrite(And) etc and it should probably be figured out if it is is possible and, if not (which seems to be the case), if it is possible to get it working.

For the code there are some comments how to write it more efficiently.

simplify is a bit risky to use since ideally it should simplify it back to xors...

@shubhamabhang77
Copy link
Contributor Author

Ideally it should be possible to call expr.rewrite(And) etc and it should probably be figured out if it is is possible and, if not (which seems to be the case), if it is possible to get it working.

For the code there are some comments how to write it more efficiently.

simplify is a bit risky to use since ideally it should simplify it back to xors...

    def rewrite(self, e):
        if str(e) == "Nor":
            return Nor(*[Not(arg) for arg in self.args])

This can be done.
What you say?

@oscargus
Copy link
Contributor

As I wrote in the issue thread, simply renaming the methods to _eval_rewrite_as_Nand etc should allow that. I simply suggested the wrong name initially...


def test_rewrite_as_Nand():
expr = (y & z) | (z & ~w)
assert str(expr._rewrite_as_Nand()) == '~(~(y & z) & ~(z & ~w))'
Copy link
Member

Choose a reason for hiding this comment

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

Test this directly, not using the str form.

@asmeurer
Copy link
Member

Please be more descriptive in the release notes.

@shubhamabhang77
Copy link
Contributor Author

When I use _eval_rewrite_as_Nand this in code and when I call rewrite(Nand) some problems in occuring regarding no. of arguments.

@shubhamabhang77
Copy link
Contributor Author

Please be more descriptive in the release notes.

Okay

@oscargus
Copy link
Contributor

I've read up a bit more. You need arguments as follows:
def _eval_rewrite_as_Nand(self, arg, **kwargs):

where arg is the arguments to use (I assume it may be the same as self.args, but not sure).

Also, Or(*[_convert_to_varsSOP(x, arg) for x in list(filter(lambda x: sum(x)%2, ibin(len(arg), 'all')))]) should do the trick to convert the an Xor to And/Or.

@shubhamabhang77
Copy link
Contributor Author

shubhamabhang77 commented Mar 12, 2019

I've read up a bit more. You need arguments as follows:
def _eval_rewrite_as_Nand(self, arg, **kwargs):

where arg is the arguments to use (I assume it may be the same as self.args, but not sure).

Also, Or(*[_convert_to_varsSOP(x, arg) for x in list(filter(lambda x: sum(x)%2, ibin(len(arg), 'all')))]) should do the trick to convert the an Xor to And/Or.

What should be the given arguments when we are going to call .rewrite(Nand)? Where did you read about it?

@oscargus
Copy link
Contributor

I just looked at other _eval_rewrite_as_XX methods in the source and tried to figure out how they did it.

@shubhamabhang77
Copy link
Contributor Author

I just looked at other _eval_rewrite_as_XX methods in the source and tried to figure out how they did it.

What did you find?

@oscargus
Copy link
Contributor

What those arguments are. There are plenty of these functions around so look at one.

@oscargus
Copy link
Contributor

I've updated this now. Hopefully the tests will pass. (I did it on the original branch and as I'm not very fluent with that I wasn't able to run the tests locally...)

@oscargus oscargus added PR: sympy's turn and removed PR: author's turn The PR has been reviewed and the author needs to submit more changes. labels Jun 25, 2019
@oscargus
Copy link
Contributor

As there is a risk that this is not seen in a while by potential mergers, being a rather old PR: ping @oscarbenjamin @smichr @asmeurer

def _eval_rewrite_as_And(self, *args, **kwargs):
a = self.args
return And(*[_convert_to_varsPOS(x, self.args)
for x in _get_even_parity_terms(len(a))])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these two functions converting to CNF/DNF (SOP/POS)? If so is that not already implemented?

Copy link
Contributor

Choose a reason for hiding this comment

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

See this comment: #16194 (comment)

Basically, it has to do with the expected form. Rewrite rewrites using a closed form expression, keeping sub-expressions, while to_cnf/to_nnf tried to simplify the expression (to some extent) and keeping everything on an SOP/POS form (but not simplifying it to the minimal SOP/POS form). I think both have their use.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds like you're more qualified than me to merge this :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

My only comment would be that it would be good to add tests that demonstrate the similarity/difference between this and the related methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can do that in another PR where I can more easily run the tests locally.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have the following in my .git/config:

[remote "upstream"]
    url = https://github.com/sympy/sympy.git
    fetch = +refs/heads/*:refs/remotes/upstream/*
    fetch = +refs/pull/*/head:refs/remotes/upstream/pr/*

The means that git fetch upstream gives me a local copy of every PR. I can then check this out with

$ git fetch upstream
$ git checkout upstream/pr/16194

The first time you run git fetch with this is slow.

I'm still not quite sure what is the best way to push to a PR using this setup...

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the hint! Indeed a good way to try out PRs I believe.

I'll merge this for now then and submit a new PR with more tests.

@oscargus oscargus merged commit 5f13feb into sympy:master Jun 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants