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

Implemented ComplexPlane Class: Complex Sets #9463

Merged
merged 13 commits into from Jun 19, 2015

Conversation

Projects
None yet
5 participants
@aktech
Copy link
Member

aktech commented Jun 3, 2015

Initial version of ComplexPlane Class

@flacjacket @hargup

TODO:

  • ComplexPlane definition
  • Add rules for Intersection & Union with Real Sets
  • normalize_theta_set
  • Add Tests
  • Add Printer
  • 100% Coverage

Other TODO: (Maybe in an another PR)

  • Special Printer for ComplexPlane (with custom Intervals)
@hargup

This comment has been minimized.

Copy link
Member

hargup commented Jun 3, 2015

+1 for the doc-strings.

It will be better if you specify input in polar form by keyword arguments like polar=True rather than the type of input.

@aktech

This comment has been minimized.

Copy link
Member Author

aktech commented Jun 3, 2015

It will be better if you specify input in polar form by keyword arguments like polar=True rather than the type of input.

Done.

@@ -468,8 +468,8 @@ def test_sympy__sets__sets__Complement():

def test_sympy__sets__sets__SymmetricDifference():
from sympy.sets.sets import FiniteSet, SymmetricDifference
assert _test_args(SymmetricDifference(FiniteSet(1, 2, 3), \
FiniteSet(2, 3, 4)))
assert _test_args(SymmetricDifference(FiniteSet(1, 2, 3),

This comment has been minimized.

@hargup

hargup Jun 3, 2015

Member

Please avoid pure formatting changes, they mess up git-blame.

This comment has been minimized.

@aktech

aktech Jun 3, 2015

Author Member

It wasn't pure formatting though, I have added args tests for ComplexPlane Class, in that file.
I read somewhere in mailing list discussion by jcrist , It's good to make formatting changes around the code we are playing with. Isn't it?

This comment has been minimized.

@toolforger

toolforger Jun 3, 2015

Contributor

I agree that formatting can be done if the code is changed anyway; however, in this case

  1. You aren't changing the code near these lines (though you do remove a superfluous backslash),
  2. the new formatting is not an actual improvement (both ways to format the code are PEP8-compliant, but the changed formatting style is sometimes considered less good).

Strictly speaking for myself and not for the project, I'd recommend keeping subexpressions on one line as far as possible, and indent them by multiples of 4. Something like this:

assert _test_args(
    SymmetricDifference(FiniteSet(1, 2, 3), FiniteSet(2, 3, 4)))

Still, this isn't really a change related to your other work; I wouldn't -1 the PR if it is in, but I'd recommend keeping formatting away from "real work" so that people don't need to scroll over it when they're looking for what's in the PR description.

This comment has been minimized.

@aktech

aktech Jun 3, 2015

Author Member

Thanks, removed formatting!

@aktech aktech force-pushed the aktech:complex-sets branch 2 times, most recently from 05c8dc5 to 003e7af Jun 3, 2015

@flacjacket flacjacket added the GSoC label Jun 4, 2015

a, b = self.args[1].args

# self in rectangular form
if lambda_arg.is_Add:

This comment has been minimized.

@hargup

hargup Jun 4, 2015

Member

Use the more descriptive self.polar to check the form of the representation.

This comment has been minimized.

@aktech

aktech Jun 4, 2015

Author Member

Sure.

return ImageSet.__new__(cls, Lambda((r, theta),
r*(cos(theta) + I*sin(theta))), a*b)

def _contains(self, other):

This comment has been minimized.

@hargup

hargup Jun 4, 2015

Member

As @moorepants suggested in #9438 (comment), it is recommended that you write the tests before you write the code for a particular feature, it will the expectations from the code clear and it will also make it easier for us to test if your code is correct or not.

I saw that you have written doctests which partially solves the purpose but unit tests gives a clearer picture of the behaviour of code.

This comment has been minimized.

@aktech

aktech Jun 4, 2015

Author Member

Yes, I will keep that in mind from now.

This comment has been minimized.

@aktech

aktech Jun 7, 2015

Author Member

I have added some tests now.

@@ -443,3 +444,126 @@ def _sup(self):
@property
def _boundary(self):
return self


def as_r_theta(z):

This comment has been minimized.

@hargup

hargup Jun 4, 2015

Member

We don't need this function, we already have arg 1 and Abs 2 as a part of elementary functions.

This comment has been minimized.

@aktech

aktech Jun 4, 2015

Author Member

Yes, thanks!

@aktech aktech force-pushed the aktech:complex-sets branch 3 times, most recently from 201c477 to 650e21a Jun 7, 2015

assert upper_half_plane.intersect(lower_half_plane) == EmptyPlane

# Intersection Polar and Rectangular
raises(ValueError, lambda: unit_disk.intersect(unit_square))

This comment has been minimized.

@hargup

hargup Jun 8, 2015

Member

It should return an unevaluated Intersection object rather raising an error.

@leosartaj

This comment has been minimized.

Copy link
Member

leosartaj commented Jun 9, 2015

It would be good if user could do something like

>>> ComplexPlane((2, 3), (4, 6)) 

Rather than

>>> a = Interval(2, 3)
>>> b = Interval(4, 6)
>>> ComplexPlane(a, b)

Maybe args can be handled as Interval.
I would like to not import Interval, everytime I have to make an instance of ComplexPlane.

@aktech

This comment has been minimized.

Copy link
Member Author

aktech commented Jun 10, 2015

@leosartaj
With the current API, your suggestions could have been incorporated, but the current User Interface is not good, as it will not suffice in representing all possible regions in a Complex Plane, So I am in a process of changing the input API, which is ProductSets or Union of ProductSets and it's probably not good to simplify UI in that case.

@aktech

This comment has been minimized.

Copy link
Member Author

aktech commented Jun 10, 2015

@hargup @flacjacket Please have a look.

return self.args[1]

@property
def psets(self):

This comment has been minimized.

@hargup

hargup Jun 10, 2015

Member

The sets function should be enough, it is trivial to convert between psets and sets.

This comment has been minimized.

@aktech

aktech Jun 10, 2015

Author Member

I was expecting this, but it saves writing same lines of code again and again further.
psets gives usable form of the sets.

This comment has been minimized.

@hargup

hargup Jun 10, 2015

Member

Same for a_interval and b_interval, they are not really needed and you should only one function that gives the input set.

This comment has been minimized.

@aktech

aktech Jun 10, 2015

Author Member

Same as this comment.
Increases readability and simplifies code further in other methods.

return b_interval

@property
def polar(self):

This comment has been minimized.

@hargup

hargup Jun 10, 2015

Member

You can also assign self.polar to the polar argument in the __new__ method, also the calculation is wrong because as both polar and cartesian form takes the input as Product sets.

This comment has been minimized.

@aktech

aktech Jun 10, 2015

Author Member

You can also assign self.polar to the polar argument in the new method,

That would not work, as that creates a class variable & it gets overridden every-time a new instance is called.
That's why I have used an OOP style property.

also the calculation is wrong because as both polar and cartesian form takes the input as Product sets.

No, Since the self here is the returned object, the arg is as follows:

self.args[0].args[1] is x + I*y (Add Object) for rectangular.

self.args[0].args[1] is r*(I*sin(theta) + cos(theta)) (Mul object) for polar.

# self in rectangular form
if self.polar is False:
for element in self.psets:
if (S.Zero, S.Zero) in element:

This comment has been minimized.

@hargup

hargup Jun 10, 2015

Member

You only need to check if the imaginary part is zero, real part need not be zero.

This comment has been minimized.

@aktech

aktech Jun 10, 2015

Author Member

Yes, thanks!

# Polar Form
elif polar is True:
from sympy import cos, sin
obj = ImageSet.__new__(cls, Lambda((r, theta),

This comment has been minimized.

@hargup

hargup Jun 10, 2015

Member

Theta should be normalized to be in set [0, pi).

This comment has been minimized.

@aktech

aktech Jun 10, 2015

Author Member

Sorry, I have not documented that, but I am expecting theta to be in [0, 2*Pi) .

This comment has been minimized.

@hargup

hargup Jun 10, 2015

Member

Sorry my bad, it is [0, 2*Pi)

This comment has been minimized.

@aktech

aktech Jun 11, 2015

Author Member

I think we should raise a ValueError , when theta is not in [0, 2*Pi) , with a message " Please enter theta in the range [0, 2*Pi) "
So, that we can avoid pre-processing of theta & avoid confusion as well.
thoughts?

This comment has been minimized.

@hargup

hargup Jun 11, 2015

Member

Nope ValueError won't be correct as theta outside [0, 2*pi) are also valid values. You need to express theta in form of 2*n*pi + x where n is an integer and x is a value in [0, 2*pi) and use that x. As far as I remember there is a method in the Trigonometry module which does this normalization thing.

This comment has been minimized.

@aktech

aktech Jun 11, 2015

Author Member

Got it, _pi_coeff does that.
Thanks!

# self in polar form
elif self.polar:
for element in self.psets:
if (0 in element.args[1]) or (S.Pi in element.args[1]):

This comment has been minimized.

@hargup

hargup Jun 10, 2015

Member

You need not check for Pi if theta is normalized. If it is not normalized then you will have to check for all n*Pi where n is any integer.

This comment has been minimized.

@aktech

aktech Jun 10, 2015

Author Member

See this.

return ComplexPlane(new_r_interval*new_theta_interval,
polar=True)

if other.is_Interval:

This comment has been minimized.

@hargup

hargup Jun 10, 2015

Member

So the general idea is to take real values in the complex set and take their intersection with the real set provided. In that case you real set need not be an Interval, it can be any general real set.

This comment has been minimized.

@aktech

aktech Jun 10, 2015

Author Member

What are the other forms expected?
Union ?

This comment has been minimized.

@hargup

hargup Jun 10, 2015

Member

It can be anything Union, Intersection or ImageSet, You need not check for the type of other you can simply do other in S.Reals.

This comment has been minimized.

@aktech

aktech Jun 10, 2015

Author Member

OK, thanks!

@hargup hargup referenced this pull request Jun 12, 2015

Closed

Complex Class Added #8893

2 of 3 tasks complete

@aktech aktech force-pushed the aktech:complex-sets branch from 19d4863 to d3b9fb3 Jun 13, 2015

@aktech

This comment has been minimized.

Copy link
Member Author

aktech commented Jun 13, 2015

Changed normalize_theta_Interval to normalize_theta_set.
It now accepts FiniteSet & Interval.

raise NotImplementedError("Normalizing theta when, its %s is not"
"Implemented" % type(theta))
else:
raise ValueError(" %s does not a belongs to S.Reals" % (theta))

This comment has been minimized.

@hargup

hargup Jun 13, 2015

Member

Better say %s is not a real set, none of the sets, including Intervals, belongs to S.Reals as its members are only numbers.

assert c7.union(c8) == ComplexPlane(p4)


def test_normalize_theta_set():

This comment has been minimized.

@hargup

hargup Jun 13, 2015

Member

Also add a test to check if it raises a ValueError for non real sets.

@hargup

This comment has been minimized.

Copy link
Member

hargup commented Jun 13, 2015

normalize_theta_set is fine. Can you include a coverage report for your code?

@aktech

This comment has been minimized.

Copy link
Member Author

aktech commented Jun 13, 2015

coverage-complex-sets

@aktech aktech force-pushed the aktech:complex-sets branch from b4f17d1 to 2defe79 Jun 18, 2015

new_theta.append(k*S.Pi)
# one complete circle
if (new_theta[0] == new_theta[1]):
return Interval(0, 2*S.Pi, False, True)

This comment has been minimized.

@hargup

hargup Jun 18, 2015

Member

This line is not covered.

This comment has been minimized.

@aktech

aktech Jun 18, 2015

Author Member

I just noticed, these two lines aren't needed anymore, since we already covered the case of complete circle above in Line 493. I would remove these.

psets = ()
psets = psets + (self.args[1], )
else:
psets = self.args[1].args

This comment has been minimized.

@hargup

hargup Jun 18, 2015

Member

This line is not covered.

@hargup

This comment has been minimized.

Copy link
Member

hargup commented Jun 18, 2015

@aktech run ./bin/coverage_report.py sympy/sets to generate the html report, if click on the module it will show the lines not covered in red. Except 4 lines, your code is fully covered by your test suite, which is cool thing. Adding tests for those lines should be trivial. After that, I'm +1 to for a merge.

new_interval.append(element.args[0])
new_interval = Union(*new_interval)
return Intersection(new_interval, other)
return None

This comment has been minimized.

@hargup

hargup Jun 18, 2015

Member

This and this is also not covered. Tests for these should check if we are getting an unevaluated Union or Intersection object if the evaluation fails.

This comment has been minimized.

@aktech

aktech Jun 18, 2015

Author Member

I am adding tests for unevaluated Union or Intersection object if the evaluation fails, but we don't need return None here for _intersect method, as unevaluated object is already returned by the call to Intersection class.

@aktech

This comment has been minimized.

Copy link
Member Author

aktech commented Jun 18, 2015

Coverage of ComplexPlane: 100%

complexplane-coverage

@aktech

This comment has been minimized.

@hargup hargup added sets and removed PR: author's turn labels Jun 19, 2015

@@ -177,7 +179,7 @@ def test_fun():
Range(-10, 11))) == FiniteSet(-1, -sqrt(2)/2, 0, sqrt(2)/2, 1))


def test_reals():
def test_Reals():

This comment has been minimized.

@hargup

hargup Jun 19, 2015

Member

This is unrelated to this PR, please avoid such changes from the next time.

hargup added a commit that referenced this pull request Jun 19, 2015

Merge pull request #9463 from aktech/complex-sets
Implemented ComplexPlane Class: Complex Sets

@hargup hargup merged commit 4b98775 into sympy:master Jun 19, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hargup

This comment has been minimized.

Copy link
Member

hargup commented Jun 19, 2015

@aktech Good work 👍 , it is in now.

@aktech

This comment has been minimized.

Copy link
Member Author

aktech commented Jun 19, 2015

Thanks @hargup !

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