Simplify function of Sum and Product on simplify #22278
Conversation
|
✅ Hi, I am the SymPy bot (v167). 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.12. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
oscargus
force-pushed
the
sumproductsimplify
branch
from
599fb80
to
4b1fef7
Compare
|
The error is caused by factoring out an expression that should not be factored out...
Note that the "constant" in front of the right summation is dependent on Y. For some reason this is now factored out. |
oscargus
force-pushed
the
sumproductsimplify
branch
from
4b1fef7
to
e629d04
Compare
|
Seems like the problem causing the failed test was fixed elsewhere. |
|
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
[41d90958] [a3826118]
- 996±1μs 622±1μs 0.63 solve.TimeSparseSystem.time_linear_eq_to_matrix(10)
- 2.85±0.01ms 1.15±0ms 0.40 solve.TimeSparseSystem.time_linear_eq_to_matrix(20)
- 5.68±0.01ms 1.69±0ms 0.30 solve.TimeSparseSystem.time_linear_eq_to_matrix(30)
Full benchmark results can be found as artifacts in GitHub Actions |
sympy/simplify/simplify.py
Outdated
| for term in prod_terms: | ||
| f = term.function.simplify(**kwargs) | ||
| if f.is_zero: | ||
| return S.Zero |
There was a problem hiding this comment.
Thee probably needs to be some checking here:
In [4]: p = Product(Product(sin(t)**2+cos(t)**2-1, (k, 0, 1))*Product(1/k, (k, 0, 1)), (n, 0, 1))
In [5]: p
Out[5]:
1
─┬───────────┬─
│ │ 1 1
│ │ ─┬────┬─ ─┬──┬─
│ │ │ │ 1 │ │ 2 2
│ │ │ │ ─⋅ │ │ sin (t) + cos (t) - 1
│ │ │ │ k │ │
│ │ │ │ k = 0
│ │ k = 0
│ │
n = 0
In [6]: simplify(p)
Out[6]: 0
In [7]: p.doit()
Out[7]:
4
⎛ 2 2 ⎞
zoo⋅⎝sin (t) + cos (t) - 1⎠
In [8]: p.doit().simplify()
Out[8]: nanThere was a problem hiding this comment.
In which way should they evaluate? But of course it is possible to skip the early exit.
There was a problem hiding this comment.
The most important thing is that we don't get a definite-looking result for a case like this that is really undefined.
There was a problem hiding this comment.
I came back to this now and it turns out that the problem is actually not in this implementation (although return 0 is not a good idea).
In current master:
In [3]: p = Product(sin(t)**2+cos(t)**2-1, (k, 0, 1))*Product(1/k, (k, 0, 1))
In [4]: p.simplify()
Out[4]: 0
In [5]: p.doit()
Out[5]: zoo*(sin(t)**2 + cos(t)**2 - 1)**2
There was a problem hiding this comment.
Which seems to come from:
In [3]: p = 0*Product(1/k, (k, 0, 1))
In [4]: p
Out[4]: 0
There was a problem hiding this comment.
I think this comes from:
Lines 684 to 691 in a382611
where especially is_finite == False actually should be is_finite != True, but that will also give some other interesting effects, like 0*x not evaluating to 0 with default assumptions.
There was a problem hiding this comment.
Nah, that should rather be something like:
# we know for sure the result will be 0 except the multiplicand
# is infinity or a matrix
if any(isinstance(c, MatrixExpr) for c in nc_part):
return [coeff], nc_part, order_symbols
if any(c.is_finite == False for c in c_part):
return [S.NaN], [], order_symbols
possibly_not_finite = [c for c in c_part if c.is_finite == None]
return [coeff], possibly_not_finite, order_symbolsThere was a problem hiding this comment.
Changing this leads to that the code breaks hard. Not only test failures, but test errors. Will update once everything is ran.
There was a problem hiding this comment.
Previous was wrong. Correct is probably:
# we know for sure the result will be 0 except the multiplicand
# is infinity or a matrix
if any(isinstance(c, MatrixExpr) for c in nc_part):
return [coeff], nc_part, order_symbols
if any(c.is_finite == False for c in c_part):
return [S.NaN], [], order_symbols
c_part = [c for c in c_part if c.is_finite == None]
if not c_part:
return [coeff], [], order_symbolsBut quite a lot of failures and simplify still evaluates incorrectly to 0.
oscargus
force-pushed
the
sumproductsimplify
branch
5 times, most recently
from
b1d743b
to
2021776
Compare
|
Looks good. |
References to other Issues or PRs
Relates to #22260 (also see #22264 which applies simplification as part of
doitwhich also makes sense)Closes #14219 (just adding test, fixed elsewhere)
Brief description of what is fixed or changed
The function in the
SumandProductis simplified when callingsimplify.Also,
product_simplifyis improved a bit, honoring simplify keyword arguments and usingsift.Other comments
Release Notes
SumandProductis now simplified when callingsimplify.