sets - fixes imageset does not work over range #21794
Conversation
|
✅ Hi, I am the SymPy bot (v161). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.
Click here to see the pull request description that was parsed. |
praneethratna
changed the title
|
Benchmark results from GitHub Actions Lower numbers are good, higher numbers are bad. A ratio less than 1 Significantly changed benchmark results (PR vs master) Significantly changed benchmark results (master vs previous release) before after ratio
[ed9a550f] [12b7787c]
<sympy-1.8^0>
- 8.83±0s 4.25±0s 0.48 integrate.TimeIntegrationRisch02.time_doit(100)
- 8.76±0s 4.24±0s 0.48 integrate.TimeIntegrationRisch02.time_doit_risch(100)
+ 73.5±0.1μs 2.57±0.01ms 35.00 matrices.TimeDiagonalEigenvals.time_eigenvals
- 5.10±0.01ms 2.81±0ms 0.55 solve.TimeRationalSystem.time_linsolve(10)
- 1.03±0ms 653±1μs 0.64 solve.TimeRationalSystem.time_linsolve(5)
- 1.21±0ms 795±2μs 0.66 solve.TimeSparseSystem.time_linsolve_eqs(10)
- 2.27±0.01ms 1.44±0ms 0.64 solve.TimeSparseSystem.time_linsolve_eqs(20)
- 3.33±0ms 2.12±0ms 0.64 solve.TimeSparseSystem.time_linsolve_eqs(30)
Full benchmark results can be found as artifacts in GitHub Actions |
| try: | ||
| n = self.size | ||
| except ValueError: | ||
| n = None |
There was a problem hiding this comment.
I would prefer an explicit check rather than catching an exception.
There was a problem hiding this comment.
@oscarbenjamin i mean like I've tried an explicit check ie something like
if not self.size.is_Integer:
return
but it kinda seems to return the initial error of the problem ie the ValueError('invalid method for symbolic range'),
what could be a possible solution to this?
There was a problem hiding this comment.
Perhaps Range could be modified to return None instead of an error -- but I'm not sure what the implications of that would be. Capturing the ValueError for a single line of code that queries a property that raises a ValueError if the size is unknown doesn't bother me so much, however. (But I'm the one that suggested it.)
There was a problem hiding this comment.
There should be a way to get the symbolic size. It could be a different attribute like r.length or something but the size of Range(n) is n and it should be possible to get that in some way without an exception.
There was a problem hiding this comment.
Only a few symbolic assumptions could actually give you a size (like even+prime=2). And it is worse with Range(x,y,z). Symbolic ranges are largely for display purposes only; is it worth the work to return a Piecewise that would be true for symbolic arguments? And that should only be returned if all args are declared as integers. It's not a trivial matter; I doubt it is of any significant use; I would prefer to just let the ValueError signal that it's not available (as here) and let the code work around it. Would returning None be better than raising an error? Or would NotImplemented('on purpose') be better?
There was a problem hiding this comment.
If we are going to have a Range with symbolic start/end then its size can be symbolic as well. I can see the value in having an attribute that gives the size when it is known as an explicit integer and raises otherwise (as .size does). There also is a need for an attribute that simply gives the symbolic size as an expression e.g. Range(n).length -> n even if n is a symbol or some other expression.
Many builtin operations can raise ValueError so ValueError should not be explicitly raised and caught within the SymPy codebase because it can mask bugs. If there is ever a need to catch the exception then a different exception class should be raised in the first place.
It is more robust not to catch exceptions at all though. There should be an interface that makes it possible to check whether an operation should succeed without needing to catch an exception: then any exception that does get raised can bubble up and can rightly be considered a bug.
There was a problem hiding this comment.
This is a test of the Piecewise logic for the step size (assuming 0 for cases that would raise an error):
from itertools import combinations, permutations
for i in combinations(range(-3,4),3):
for i in permutations(i):
start,stop,step=i
if all([int(start)==start,int(stop)==stop,int(step)==step,(stop-start)*step>0]):
n = int((stop - start)/step) + (1 if (stop-start)%step else 0)
else:
n=0
try:
m=len(range(*i))
except:
m=0
assert n == mAnd here is the Piecewise which checks that all args are integers or at most 1 of them is infinite, that they are compatible with the direction of the step:
Piecewise((floor((stop - start)/step) + Piecewise((1,Ne(Mod(stop-start,step),0)),
(0,True)),And(Eq(floor(start),start),Eq(floor(stop),stop),Eq(floor(step),step),(stop -
start)*step>0,Ne(step+1,step),Or(Ne(start+1,start),Ne(stop+1,stop)))),(0,True))There was a problem hiding this comment.
Okay, that's very complicated. That makes me wonder if symbolic range was a good idea. We mainly use it because we want to represent sets of integers but it would be better to have something simpler for that e.g.: #17856 (comment)
There was a problem hiding this comment.
We supported it for the purpose of code generation. We could also compute the size for something like Range(i, i + k*j, k) as j but I think for simplicity we just steered clear of trying to support it without a well defined need.
smichr
changed the title
|
Closing this as the alternative fix in #21932 has been merged. |
References to other Issues or PRs
Brief description of what is fixed or changed
Fixes #18400 by changing code in sympy/sets/handlers/functions.py and also added a test case function test_issue_18400() in sympy/sets/tests/test_sets.py
Other comments
Release Notes
NO ENTRY