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

[WIP] Allowing integer symbols in Range #17146

Open
wants to merge 15 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@czgdp1807
Copy link
Member

commented Jul 3, 2019

References to other Issues or PRs

[1] #16833

Brief description of what is fixed or changed

This PR aims at allowing integer symbols i.e, the symbols with assumptions, integer=True to Range.

Other comments

I am planning to work step-by-step. I have started the work by updating the Range.__new__ for integer symbols and writing associated tests for it.
Subsequent methods will be updated after completing the work on previous ones.
ping @smichr @asmeurer @Upabjojr

Work in Progress

The following methods are to be changed. Those marked have been completed.

  • __new__
  • reversed
  • _contains
  • __iter__
  • __len__
  • size
  • __nonzero__
  • __getitem__
  • _inf
  • _sup
  • _boundary

Issues To Be Fixed

I will fix the issues listed below in this PR only. This is just for tracking purpose so that I don't forget fixing them.

[1] #17146 (comment)
[2] #17146 (comment)

Release Notes

  • sets
    • Integer symbols allowed in Range.
@sympy-bot

This comment has been minimized.

Copy link

commented Jul 3, 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. -->
[1] https://github.com/sympy/sympy/issues/16833

#### Brief description of what is fixed or changed
This PR aims at allowing integer symbols i.e, the symbols with assumptions, `integer=True` to Range.

#### Other comments
I am planning to work step-by-step. I have started the work by updating the `Range.__new__` for integer symbols and writing associated tests for it.
Subsequent methods will be updated after completing the work on previous ones.
ping @smichr @asmeurer @Upabjojr 

**Work in Progress**

The following methods are to be changed. Those marked have been completed.

- [x] `__new__`
- [x] `reversed`
- [x] `_contains`
- [x] `__iter__`
- [x] `__len__`
- [x] `size`
- [x] `__nonzero__`
- [ ] `__getitem__`
- [x] `_inf`
- [x] `_sup`
- [x] `_boundary`

**Issues To Be Fixed**

I will fix the issues listed below in this PR only. This is just for tracking purpose so that I don't forget fixing them.

[1] https://github.com/sympy/sympy/pull/17146#issuecomment-511806179
[2] https://github.com/sympy/sympy/pull/17146#issuecomment-511856601

#### 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 -->
* sets
  * Integer symbols allowed in `Range`.
<!-- END RELEASE NOTES -->

@codecov

This comment has been minimized.

Copy link

commented Jul 3, 2019

Codecov Report

Merging #17146 into master will increase coverage by 0.012%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##            master   #17146       +/-   ##
============================================
+ Coverage   74.507%   74.52%   +0.012%     
============================================
  Files          623      623               
  Lines       161593   161602        +9     
  Branches     37925    37926        +1     
============================================
+ Hits        120399   120426       +27     
+ Misses       35849    35834       -15     
+ Partials      5345     5342        -3
Show resolved Hide resolved sympy/sets/fancysets.py
@@ -295,6 +295,12 @@ def test_Range_set():
assert r.inf == rev.inf and r.sup == rev.sup
assert r.step == -rev.step

# test symbolic Range
n = symbols('n', integer=True)
r = Range(1, n)

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jul 3, 2019

Contributor

what happens if you call .doit()?

This comment has been minimized.

Copy link
@czgdp1807

czgdp1807 Jul 4, 2019

Author Member

It remains un-evaluated, if any of start, stop, step is a symbol.
Code

from sympy import Range, symbols
n = symbols('n', integer=True)
r1, r2, r3 = Range(n, 10), Range(1, n), Range(1, 5, n)
print(r1.doit(), r2.doit(), r3.doit())

Output

Range(n, 10, 1) Range(1, n, 1) Range(1, n*ceiling(4/n) + 1, n)

This comment has been minimized.

Copy link
@czgdp1807

czgdp1807 Jul 4, 2019

Author Member

As is apparent, things don't go well, when step is symbolic. It needs to be fixed before moving forward.
Code

from sympy import Range, symbols
n = symbols('n', integer=True)
r3 = Range(1, 5, n)
for i in range(-2, 7):
    if i == 0:
        continue
    print(i, r3.subs(n, i))

Output

-2 Range(0, 0, 1)
-1 Range(0, 0, 1)
1 Range(1, 5, 1)
2 Range(1, 5, 2)
3 Range(1, 7, 3)
4 Range(1, 5, 4)
5 Range(1, 6, 5)
6 Range(1, 7, 6)

This comment has been minimized.

Copy link
@czgdp1807

czgdp1807 Jul 4, 2019

Author Member

I think, my changes haven't done anything wrong. On running something similar on master, the following happens,
Code

from sympy import Range, symbols
for i in range(-2, 7):
    if i == 0:
        continue
    print(i, Range(1, 5, i))

Output

-2 Range(0, 0, 1)
-1 Range(0, 0, 1)
1 Range(1, 5, 1)
2 Range(1, 5, 2)
3 Range(1, 7, 3)
4 Range(1, 5, 4)
5 Range(1, 6, 5)
6 Range(1, 7, 6)

As it appears, the output is same. Well I will say, last few outputs are examples of garbage-in-garbage-out, may be it doesn't needs a fix. Or if we want to fix and want to it to behave like python range something is to be done regarding the following code,

if not start.is_infinite:
            ref = start if start.is_finite else stop
            n = ceiling((stop - ref)/step)
            if n <= 0:
                # null Range
                start = end = S.Zero
                step = S.One
            else:
                end = ref + n*step
        return Basic.__new__(cls, start, end, step)

This can be fixed by,

if not start.is_infinite:
            ref = start if start.is_finite else stop
            n = ceiling((stop - ref)/step)
            if n <= 0:
                # null Range
                start = end = S.Zero
                step = S.One
            else:
                end = ref + n*step
        if end > stop:
            end = stop 
        return Basic.__new__(cls, start, end, step)

PS:
That's how python range behaves,

Python 3.6.7 (default, Oct 22 2018, 11:32:17) 
[GCC 8.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> list(range(1, 5, 7))
[1]
>>> range(1, 5, 7)
range(1, 5, 7)

This comment has been minimized.

Copy link
@czgdp1807

czgdp1807 Jul 9, 2019

Author Member

I have observed that there is a difference between Python 3's range and Python 2's xrange. Earlier, Range was behaving like, xrange irrespective of the python version. I have made the changes to add the version check before returning the check. See, https://github.com/sympy/sympy/pull/17146/files#diff-c1c17bdd28aafd400bf9415c7d048382R566

This comment has been minimized.

Copy link
@oscarbenjamin

oscarbenjamin Jul 13, 2019

Contributor

This isn't garbage. Range canonicalises its args so that any two Ranges that would give the same numbers will have the same args. This is useful for comparing them with ==.

This comment has been minimized.

Copy link
@czgdp1807

czgdp1807 Jul 13, 2019

Author Member

I will check if my changes are affecting canonicalization of the input parameters and will do the necessary changes. It would be helpful if you can provide any example of such an affect due to my changes. Thanks.

Show resolved Hide resolved sympy/sets/fancysets.py Outdated
raise ValueError(filldedent('''
Ranges must have a literal integer step.'''))
if not step.is_finite == True:
raise ValueError("step must be a finite integer symbol.")

This comment has been minimized.

Copy link
@smichr

smichr Jul 4, 2019

Member

If you don't insist on the sign, too, being positive or negative, how will you determine which is the inf/sup?

This comment has been minimized.

Copy link
@czgdp1807

czgdp1807 Jul 4, 2019

Author Member

So, you mean that, if step is a symbol then how will we determine the inf/sup, well it's a valid point. So, how to do it? If is_positive is either True or False then it's easy. But if, is_positive is None then its hard. But if we return Piecewise something like, Piecewise((step > 0, <the-inf/sup-for-positive-case>), (step < 0, <the-inf/sup-for-negative-case>), (Eq(step, 0), <the-inf/sup-for-zero-step>)) then the problem is solved irrespective of what's the sign of step is. What do you say?

This comment has been minimized.

Copy link
@czgdp1807

czgdp1807 Jul 4, 2019

Author Member

Please refer #17146 (comment) too.

This comment has been minimized.

Copy link
@oscarbenjamin

oscarbenjamin Jul 13, 2019

Contributor

Maybe you can use Min/Max for this:

Range(a, b, c).inf -> Min(a, b-c)

Gagandeep Singh and others added some commits Jul 4, 2019

Update sympy/sets/fancysets.py
Co-Authored-By: Christopher Smith <smichr@gmail.com>
@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2019

Please provide your comments, else, I will push my changes and move on to the next method for updates. Thanks.

@Upabjojr

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

Well, do you want to push it here or in a new PR?

Regarding the sign of infinities and related problems, I'd say to make sure that any expression that cannot be clearly determined should remain unevaluated.

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

Well, do you want to push it here or in a new PR?

I will push the changes to this PR. I am working on it and trying to figure out a way to correct things with minimal changes.

Regarding the sign of infinities and related problems, I'd say to make sure that any expression that cannot be clearly determined should remain unevaluated.

I agree.

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

I have pushed the changes. Please take a look and let me know of changes. I have tried to make minimal changes.

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

Now, I am moving forward to update the next method of Range for symbolic parameters.

czgdp1807 added some commits Jul 9, 2019

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

@smichr @Upabjojr I have made some partial changes to _contains and I have observed that reversed doesn't need any changes, it works fine for symbolic parameters, so, added only tests for reversed.
I have added TODO as a reminder for myself and I will remove that before completing this PR.

Please review and let me know of any changes. I will wait for 48 hours for comments and reviews. Thanks.

else:
end = stop
# behave like Python 3 range if PY3 else behave like Python 2 xrange
if (end > stop) == True and (stop > start) == True and PY3:

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jul 11, 2019

Contributor

Maybe we don't need this. Python 3 behavior sounds better, consider that next year we're going to drop Python 2 support.

This comment has been minimized.

Copy link
@czgdp1807

czgdp1807 Jul 11, 2019

Author Member

Sure I will remove, PY3 from the above condition as soon as the PR for removing python2 support is merged into master. In fact a few more changes will be needed to remove xrange(used for python2 by Range) from other places.

This comment has been minimized.

Copy link
@oscarbenjamin

oscarbenjamin Jul 13, 2019

Contributor

Just remove this. SymPy's Range should do the same thing on all Python versions regardless of what xrange does.

This comment has been minimized.

Copy link
@czgdp1807

czgdp1807 Jul 13, 2019

Author Member

As from the reviews it is apparent that Range should behave like range, so I will make the change. However, any deprecation policy we need to follow in this case, because currently Range adapts itself to behave like range and xrange. @smichr Due you want to share any reviews on this topic?

Show resolved Hide resolved sympy/sets/tests/test_fancysets.py Outdated
Show resolved Hide resolved sympy/sets/tests/test_fancysets.py
@oscarbenjamin

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2019

It seems like Range has a lot of awkward repetitive code to prevent indexing relative to infinity:

In [4]: Range(oo)                                                                                                                 
Out[4]: {0, 1, …}

In [5]: Range(oo)[1:]                                                                                                             
Out[5]: {1, 2, …}

In [6]: Range(oo)[:-1]                                                                                                            
---------------------------------------------------------------------------
ValueError

In the symbolic case that won't necessarily work since e.g.:

Range(a)[::-1].subs(a, oo)

If we don't know that a is oo when slicing how can we detect that?

Is it really necessary to prevent slicing from infinity. I would be fine with:

In [6]: Range(oo)[:-1] 
Out[6]: {0, 1, …}
@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jul 13, 2019

I will take a look at __getitem__ for this. Thanks for your input.

czgdp1807 added some commits Jul 15, 2019

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jul 15, 2019

I have completed the _inf, _sup and _contains methods. Please take a look and let me know of any updates to be made in those methods.

@oscarbenjamin

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

The Range object here doesn't print in ipython:

In [10]: Range(k, m, n)
Out[10]: ---------------------------------------------------------------------------
...
TypeError: 'Abs' object cannot be interpreted as an integer
@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2019

It is working on mine. May be, k, m, n don't satisfy is_integer=True in your case. Details given below.

Python 3.6.7 (default, Oct 22 2018, 11:32:17) 
[GCC 8.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import IPython
>>> IPython.embed()
Python 3.6.7 (default, Oct 22 2018, 11:32:17) 
Type 'copyright', 'credits' or 'license' for more information
IPython 7.4.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: from sympy import symbols, Range                                                              

In [2]: k, m, n = symbols('k, m, n', integer=True)                                                    

In [3]: Range(k, m, n)                                                                                
Out[3]: Range(k, m, n)

In [4]:                                                                                               
Do you really want to exit ([y]/n)? y
Python 2.7.15rc1 (default, Nov 12 2018, 14:31:15) 
[GCC 7.3.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import IPython
>>> IPython.embed()
Python 2.7.15rc1 (default, Nov 12 2018, 14:31:15) 
Type "copyright", "credits" or "license" for more information.

IPython 5.5.0 -- An enhanced Interactive Python.
?         -> Introduction and overview of IPython's features.
%quickref -> Quick reference.
help      -> Python's own help system.
object?   -> Details about 'object', use 'object??' for extra details.

In [1]: from sympy import symbols, Range

In [2]: k, m, n = symbols('k, m, n', integer=True)

In [3]: Range(k, m, n)
Out[3]: Range(k, m, n)

In [4]:                                                                                               
Do you really want to exit ([y]/n)? y
@oscarbenjamin

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

Sorry I said ipython but I should have said isympy. It's because you haven't used init_printing().

$ isympy
IPython console for SymPy 1.5.dev (Python 3.6.3-64-bit) (ground types: gmpy)

These commands were executed:
>>> from __future__ import division
>>> from sympy import *
>>> x, y, z, t = symbols('x y z t')
>>> k, m, n = symbols('k m n', integer=True)
>>> f, g, h = symbols('f g h', cls=Function)
>>> init_printing()

Documentation can be found at https://docs.sympy.org/dev


In [1]: Range(oo)                                                               
Out[1]: {0, 1, …}

In [2]: Range(k, m, n)                                                          
Out[2]: ---------------------------------------------------------------------------
TypeError 
@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2019

Please provide comments on the latest changes. I have created Issues To Be Fixed in the description of this PR. Add your comment's link there if it is concerned with any bugs due to symbolic parameters. Thanks.

@oscarbenjamin

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

Now I get a new error printing in isympy:

In [1]: Range(k, m, n)                                                          
Out[1]: ---------------------------------------------------------------------------
ValueError
...
~/current/sympy/sympy.git/sympy/printing/pretty/pretty.py in _print_Range(self, s)
   1872             it = iter(s)
   1873             printset = next(it), next(it), dots
-> 1874         elif len(s) > 4:
   1875             it = iter(s)
   1876             printset = next(it), next(it), dots, s[-1]

~/current/sympy/sympy.git/sympy/sets/fancysets.py in __len__(self)
    623         if size.has(Symbol) or size.is_infinite:
    624             raise ValueError(
--> 625                 "Use .size to get the length of an infinite and symbolic Range")
    626         return int(size)
    627 

ValueError: Use .size to get the length of an infinite and symbolic Range

You should think about how this should pretty print and then fix the pretty-printing code.

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2019

Now I get a new error printing in isympy:

AFAICT, it is not possible to iterate symbolic Range since, bounds are undetermined and I believe that's the best option.
For symbolic Range, len cannot work as the length will not be an int, hence, size is recommended in the ValueError for symbolic and infinite Range.

it = iter(s)

I think this shouldn't have worked in the first place as s is a symbolic Range, if I am not wrong. I will look into this. Thanks for the code.

Regarding, printing of Range(k, m, n) I will work on it and fix it.

Edit - What I have figured out is that iter generates a generator object, and next iterates the Range which should throw an exception for symbolic bounds. Well, I think always throwing an exception isn't a good idea so, may be some improvement can be done. I am working on it.
For len then I would say using size is recommended over len for symbolic and infinite Range.

czgdp1807 added some commits Jul 17, 2019

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2019

I have made some changes to iter and the tests have also been added. You can see the snippet in the link below for the behaviour of iter. Please let me know if these changes are good or not allowing iteration over symbolic ranges is better.
https://github.com/czgdp1807/sympy/blob/6b388f3dc1f37234b3ac26b2aff01096e84da414/sympy/sets/tests/test_fancysets.py#L361-L367

@oscarbenjamin

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2019

Note: Bare next like next(it) should be avoided because it can leak a StopIteration which causes all kinds of problems. Do you know whether the list comprehension or the outer loop would catch that StopIteration or what it would do to the test runner? Use something like next(it, None) to be sure that doesn't happen.

In any case symbolic range should not be iterable if you don't know exactly how many items it should yield. There are cases where it could make sense e.g. Range(n, n+6) but it's probably better just to disallow it in all cases for now.

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2019

In any case symbolic range should not be iterable if you don't know exactly how many items it should yield. There are cases where it could make sense e.g. Range(n, n+6) but it's probably better just to disallow it in all cases for now.

So, what about range with int sizes? In that case we would be knowing the exact number of times we need to yield? That can be a middle option for us right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.