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

Created a Set Difference class #7462

Merged
merged 2 commits into from
Aug 14, 2014
Merged

Created a Set Difference class #7462

merged 2 commits into from
Aug 14, 2014

Conversation

hargup
Copy link
Contributor

@hargup hargup commented May 3, 2014

Todo list:

  • fix is that self.complement(other) behaves as other - self and self._complement(other) behaves as self - other.
  • Complement printers
  • Rebase

@hargup hargup added the Sets label May 3, 2014
@hargup hargup self-assigned this May 3, 2014
@hargup
Copy link
Contributor Author

hargup commented May 3, 2014

@mrocklin, @skirpichev ping.


>>> from sympy import Difference, FiniteSet

>>> Difference(FiniteSet(0, 1, 2), FiniteSet(1))
Copy link
Member

Choose a reason for hiding this comment

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

Probably a blank line here will create two different blocks in the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks you are right.

@asmeurer
Copy link
Member

asmeurer commented May 3, 2014

-A should probably return Difference(UniversalSet, A).

is_Difference = True
# See if we will be representing sets in terms of differences,
# I guess we can always
# represent a set difference with Union, if yes do we know how?
Copy link
Member

Choose a reason for hiding this comment

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

This comment doesn't make any sense to me.

@asmeurer
Copy link
Member

asmeurer commented May 3, 2014

By the way, go easy on the [WIP] tags. This code could probably go in as is. It's better to merge pull requests early and just open new ones for new feature than to block a pull request on a lot of features before it gets merged.

@hargup hargup added the GSoC label May 6, 2014
@hargup hargup changed the title [WIP]Created a Set Difference class Created a Set Difference class May 7, 2014
@hargup
Copy link
Contributor Author

hargup commented May 7, 2014

-A should probably return Difference(UniversalSet, A).

This is the current behavior after the recent commits.

@asmeurer @mrocklin @skirpichev @debugger22 Please review.

@skirpichev
Copy link
Contributor

On Tue, May 06, 2014 at 10:01:16PM -0700, Harsh Gupta wrote:

 -A should probably return Difference(UniversalSet, A).

I don't sure it's mathematically correct.

@asmeurer
Copy link
Member

asmeurer commented May 7, 2014

I don't sure it's mathematically correct.

How so?

@asmeurer
Copy link
Member

asmeurer commented May 7, 2014

See https://en.wikipedia.org/wiki/Set_complement#Absolute_complement. Insomuch as a universal set can be defined (and we have one defined, so I guess for us it can be), the absolute complement of a set is U - A.

@mrocklin
Copy link
Member

mrocklin commented May 7, 2014

Seems reasonable so far. It would be nice to accompany this work with a motivating use case.

@asmeurer
Copy link
Member

asmeurer commented May 7, 2014

There are tons of sets you can't represent without this. But probably the simplest example to show would be de'Morgan's laws.

@skirpichev
Copy link
Contributor

See https://en.wikipedia.org/wiki/Set_complement#Absolute_complement.
Insomuch as a universal set can be defined (and we have one defined, so I
guess for us it can be), the absolute complement of a set is U - A.

I don't sure we really know what our ("defined") UniversalSet is. The reference
in the docstring doesn't define UniversalSet at all (but clearly shows
that this notion is paradoxal in the usual set theory).

@asmeurer
Copy link
Member

asmeurer commented May 8, 2014

I think the problems with UniversalSet are similar to the problems you get with having oo. Algebraically it makes sense most of the time, although you may have to leave some operations undefined to avoid inconsistency (in this case, probably PowerSet(UniversalSet) shouldn't work). It's also similar in the sense that oo is not a real number, but if you extend the set of real numbers somehow, then it is. SImilarly, UniversalSet is not a set, but if you allow collections that aren't sets, then it can be one of those (and set operations against those collections and sets sometimes, but not always, make sense).

@hargup hargup removed the imported label May 9, 2014
@hargup
Copy link
Contributor Author

hargup commented May 9, 2014

@asmeurer I can't figure out the reason for the travis failure. The only error is with python 3.4 build.

@debugger22
Copy link
Member

See #7456

@hargup
Copy link
Contributor Author

hargup commented May 9, 2014

@asmeurer @mrocklin @skirpichev Do you think this PR is good to merge as it is.

+1 to @asmeurer 's comment. If we disagree on what UniversalSet should be we can sort it out in issue or on mailing list. Anyway there are issue with UniversalSet, we are implicitly assuming it to be Reals. S.Reals.complement returns EmptySet().

@asmeurer
Copy link
Member

asmeurer commented May 9, 2014

I would change that so that UniversalSet is the universal set.

@hargup
Copy link
Contributor Author

hargup commented May 9, 2014

I would change that so that UniversalSet is the universal set.

Agreed. But I feel that doing that might get a bit involved, now I'm leveraging the logic coded in _complement of classes like Interval and FiniteSet, that will probably have to be moved to __sub__ procedure. Doing the changes in this PR can unnecessarily delay merging Difference.

BTW what will happen if I create a branch say B out of this branch say A, and send a pull request for B. Then if this branch A gets merged in the master. Will the diff for the pull request of B still show the changes done in A.

@skirpichev
Copy link
Contributor

On Fri, May 09, 2014 at 01:14:22PM -0700, Harsh Gupta wrote:

[1]@asmeurer [2]@mrocklin [3]@skirpichev Do you think this PR is good to
merge as it is.

I don't think we should use UniversalSet here, that's why I'm -1.

Should we wipe out the UniversalSet class at all - we can discuss
on the mailing list.

@hargup
Copy link
Contributor Author

hargup commented May 10, 2014

I don't think we should use UniversalSet here, that's why I'm -1.

I have use dUniversalSet just to define complement of A as U - A here https://github.com/sympy/sympy/pull/7462/files#diff-1c851859c57f806535bf5a6edfe8386fR140. I can't see how I can not use UniversalSet without not having a UniversalSet.

@skirpichev
Copy link
Contributor

I have use dUniversalSet just to define complement of A as U - A

Yes, and I think - that's a bad idea.

@asmeurer
Copy link
Member

Agreed. But I feel that doing that might get a bit involved

Just raise NotImplementedError for the things that are too much work.

BTW what will happen if I create a branch say B out of this branch say A, and send a pull request for B. Then if this branch A gets merged in the master. Will the diff for the pull request of B still show the changes done in A.

Yes, it should work, as long as you don't rebase.

I don't think we should use UniversalSet here, that's why I'm -1.

Would you rather the Reals be the universal set?

I think we should keep UniversalSet, just like we should keep oo and nan. They don't make mathematical sense with every operation, but they do make sense with some operations, and are useful in those cases.

Why exactly do you want to remove UniversalSet? It probably is better to move this to the mailing list.

@skirpichev
Copy link
Contributor

On Sun, May 11, 2014 at 07:16:52PM -0700, Aaron Meurer wrote:

Would you rather the Reals be the universal set?

I think we should keep UniversalSet, just like we should keep oo and nan.

For oo and nan we can precisely restrict their usage to operations,
that make sense.

They don't make mathematical sense with every operation, but they do make
sense with some operations, and are useful in those cases.

Why exactly do you want to remove UniversalSet? It probably is better to
move this to the mailing list.

I don't sure it's a case for UniversalSet (if do, lets
document this). This is why oo argument doesn't work for me.

Practically, I think that most of time - we want some well defined
(universe) set instead if UniversalSet, e.g. Reals or Integers (context-dependent).

@asmeurer
Copy link
Member

I would just think of UniversalSet as a standin for the actual universal set. It often does not matter what that is, as long as all your sets are subsets of that one.

For instance, having a universal set allows you to define an abstract "complement", -A. This is not only easier to keep track of than U - A, but it makes your collection of sets into a Boolean Algebra, under which &, |, and - are all very meaningful and useful operations.

I would keep UniversalSet, but be very conservative with the operations that are allowed with it (e.g., UniversalSet.contains(UniversalSet) should probably raise an error).

Are there any other computer algebra systems that can manipulate sets algebraically?

@@ -1529,33 +1571,6 @@ def _eval_imageset(self, f):
return FiniteSet(*map(f, self))

@property
def _complement(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The logic here was moved to Reals._complement.

@hargup
Copy link
Contributor Author

hargup commented Jun 29, 2014

As it should be __sub__ is now just a syntactic sugar and contains no logic, and same for .complement method for classes. All the logic of the Complement class is dispatched through the ._complement method in various classes. The only thing left to fix is that self.complement(other) behaves as other - self and self._complement(other) behaves as self - other.

@skirpichev
Copy link
Contributor

Any news?

Lets fill some todo list:

  • fix is that self.complement(other) behaves as other - self and self._complement(other) behaves as self - other. // Probably, you should fix _complement methods (private).
  • Complement printers

@mrocklin, is there something else?

@hargup
Copy link
Contributor Author

hargup commented Aug 5, 2014

@skirpichev I have fixed both issues.

@skirpichev
Copy link
Contributor

We have a doctest failure.

@skirpichev
Copy link
Contributor

And I don't think we should use the difference syntax for printing of the set complement. It seems, there are a more standard and consistent notation:
http://en.wikipedia.org/wiki/Complement_%28set_theory%29

@hargup
Copy link
Contributor Author

hargup commented Aug 8, 2014

The tests timed out.

Interval(0, 1),
((Interval(neginf, 0, True, True) + Interval(1, inf, True, True))
* (Interval(neginf, 0, True, True) + Interval(1, inf, True, True))))
# It is trouble to do this test as the result can many
Copy link
Contributor

Choose a reason for hiding this comment

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

We should fix this problem. Would complement(Interval(-oo,oo)*Interval(-oo,oo)) work?

@skirpichev
Copy link
Contributor

@mrocklin, could you take look. I'm going to merge this pr.

@hargup, we should fix mentioned above two tests.

@@ -1444,6 +1512,56 @@ def _boundary(self):
return EmptySet()


class UniversalSet(with_metaclass(Singleton, Set)):
Copy link
Contributor

Choose a reason for hiding this comment

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

We have two UniversalSet classes!

@hargup
Copy link
Contributor Author

hargup commented Aug 9, 2014

@skirpichev I merged your PR's and also rebased. The travis build also passed.

@skirpichev
Copy link
Contributor

@hargup, I'm +1. Lets wait ack from @mrocklin.

@hargup
Copy link
Contributor Author

hargup commented Aug 10, 2014

@mrocklin can we merge this PR?

@smichr
Copy link
Member

smichr commented Aug 11, 2014

needs another rebase

Sergey B Kirpichev <skirpichev@gmail.com> helped with some fixes
in the PR's #5 and
#4

* Remove duplicated UniversalSet class
* Reenable some old tests
* Fix random test failure

Conflicts:
	sympy/sets/tests/test_sets.py
@hargup
Copy link
Contributor Author

hargup commented Aug 13, 2014

@skirpichev @smichr @mrocklin I have resolved the merge conflicts also the travis build passed.

@mrocklin
Copy link
Member

Sorry for being absent here. I say go ahead.

hargup added a commit that referenced this pull request Aug 14, 2014
Created a Set Difference class
@hargup hargup merged commit 6558cf3 into sympy:master Aug 14, 2014
@hargup
Copy link
Contributor Author

hargup commented Aug 14, 2014

Thanks @asmeurer @skirpichev and @mrocklin, the PR is in. I have also updated the release notes https://github.com/sympy/sympy/wiki/Release-Notes-for-0.7.6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants