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.

Sign up for GitHub

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

Jump to bottom

logical nand #1098

Merged
merged 1 commit into from Oct 26, 2016
Merged

logical nand #1098

merged 1 commit into from Oct 26, 2016

Conversation

RiteshKB
Copy link
Contributor

No description provided.

nishnik
nishnik reviewed Oct 24, 2016
View reviewed changes
symengine/tests/logic/test_logic.cpp
REQUIRE(eq(*logical_nand({boolFalse}), *boolTrue));

REQUIRE(eq(*logical_nand({boolTrue, boolFalse}), *boolTrue));

Copy link
Contributor

Choose a reason for hiding this comment

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

This line is not needed

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 have removed that extra test.

@isuruf
Copy link
Member

isuruf commented Oct 25, 2016

What does SymPy do for Nand?

@RiteshKB
Copy link
Contributor Author

@isuruf i have added some tests with the help of sympy.

@isuruf
Copy link
Member

isuruf commented Oct 25, 2016

Looking at the PR, the class Nand is not used anywhere, I would just remove it.

@isuruf
Copy link
Member

isuruf commented Oct 25, 2016

I looked at what SymPy does and it immediately returns Not(And(x, y)) instead of Nand(x, y).
I think SymPy does this because Not(And(x, y)) gives more information and will be helpful in automatic simplifying. So, a separate class Nand is not needed at all.

isuruf
isuruf reviewed Oct 25, 2016
View reviewed changes
symengine/logic.cpp
@@ -354,6 +405,12 @@ RCP<const Boolean> logical_and(const set_boolean &s)
return and_or<And>(s, false);
}

RCP<const Boolean> logical_nand(const set_boolean &s)
{
RCP<const Boolean> a = and_or<Nand>(s, false);
Copy link
Member

Choose a reason for hiding this comment

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

This should be,
RCP<const Boolean> a = and_or<And>(s, false);

Copy link
Member

Choose a reason for hiding this comment

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

Or simply RCP<const Boolean> a = logical_and(s);

@isuruf
Copy link
Member

isuruf commented Oct 25, 2016 •
edited

@RiteshKB, your system time seems to be wrong. Can you fix it and edit the commit?

EDIT: At the time of this post, it is Tue Oct 25 15:40:00 2016 +0530

$ git show --format=fuller
commit 32f31a7a8e0e9e2af92f4525ad7fa4426e4102d4
Author:     comperror <babarwal.ritesh@gmail.com>
AuthorDate: Tue Oct 25 07:27:08 2016 +0530
Commit:     comperror <babarwal.ritesh@gmail.com>
CommitDate: Tue Oct 25 19:51:27 2016 +0530

@RiteshKB
Copy link
Contributor Author

@isuruf what else need to be done?

@isuruf
Copy link
Member

isuruf commented Oct 26, 2016

Looks good

@isuruf isuruf merged commit 2f5853b into symengine:master Oct 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nishnik nishnik nishnik left review comments

@isuruf isuruf isuruf approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@RiteshKB @isuruf @nishnik