Opened 6 years ago
Closed 5 years ago
#15346 closed defect (fixed)
Symbolic sums should evaluate
Reported by:  kcrisman  Owned by:  

Priority:  major  Milestone:  sage6.5 
Component:  symbolics  Keywords:  
Cc:  mjo  Merged in:  
Authors:  Ralf Stephan  Reviewers:  KarlDieter Crisman 
Report Upstream:  N/A  Work issues:  
Branch:  3ee8c6d (Commits)  Commit:  3ee8c6d29ce916b70e5c0f32a0ae961796344afd 
Dependencies:  Stopgaps: 
Description (last modified by )
This ask.sagemath question points out that
sage: k,n = var('k,n') sage: f(x,k) = sum((2/n)*(sin(n*x)*(1)^(n+1)), n, 1, k) sage: f(x,2) 2*sum((1)^n*sin(n*x)/n, n, 1, 2)
while
sage: f(x)=(2/n)*(sin(n*x)*(1)^(n+1)) sage: sum(f, n, 1, 2) #using summation function sin(2*x) + 2*sin(x)
User twch found this workaround
sage: var('n') sage: def g(x,k): sage: return sum((2/n)*(sin(n*x)*(1)^(n+1)), n, 1, k) sage: print g(x,2) sin(2*x) + 2*sin(x)
but I agree with him/her that we should look into how to fix this.
The essential problem is that when Maxima does not simplify a sum, we don't have any mechanism (currently) to get it to "just print out all the numbers". Which of course may not be very nice when k
is big, but presumably should be allowed to be done by users.
By the way, the way to do this in Maxima is as follows:
(%i1) f: 2*'sum((1)^n*sin(n*x)/n,n,1,2); 2 ==== n \ ( 1) sin(n x) (%o1)  2 >  / n ==== n = 1 (%i8) f, nouns; sin(2 x) (%o8)  2 (  sin(x)) 2
so setting nouns:true
just for this would work, but I can never figure out how to do this from within Sage  see #10955.
Possibly related: #9424
See also
Change History (34)
comment:1 Changed 6 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:2 Changed 6 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:3 Changed 5 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:4 Changed 5 years ago by
 Description modified (diff)
 Priority changed from minor to major
 Summary changed from symbolic sums that don't simplify don't simplify to Symbolic sums should evaluate
comment:5 Changed 5 years ago by
 Branch set to u/rws/symbolic_sums_should_evaluate
comment:6 Changed 5 years ago by
 Commit set to df516f88d0c13d22d60e8b7d191eb0b2aa75f26a
 Status changed from new to needs_review
comment:7 Changed 5 years ago by
The issue however begs the question if it should not be possible to give hold parameter to sums.
comment:8 Changed 5 years ago by
Don't have time to review this immediately (though I think the hold
would be a separate ticket, though an important one) but am wondering whether a couple of the more complex sums mentioned here and in the ask.sagemath questions should be included as tests.
comment:9 Changed 5 years ago by
 Commit changed from df516f88d0c13d22d60e8b7d191eb0b2aa75f26a to af565761f5a154fc1745bc754c3cbbb642065baf
Branch pushed to git repo; I updated commit sha1. New commits:
af56576  15346: add doctest

comment:10 Changed 5 years ago by
 Commit changed from af565761f5a154fc1745bc754c3cbbb642065baf to 37f26afb68fdf136b9568db127395f6115d22a92
Branch pushed to git repo; I updated commit sha1. New commits:
37f26af  Merge branch 'develop' into t/15346/symbolic_sums_should_evaluate

comment:11 Changed 5 years ago by
You're going to get annoyed by this question, but I do feel it's a UI issue. Is simplify_sum
the right name for this? I'm just wondering; it isn't exactly a "simplification". Though evaluate_sum
doesn't exactly get me excited either. What do you think? Also, I'm not 100% sure it should be in simplify_full
 a very long one of these might feel more complicated than "just the sum".
comment:12 Changed 5 years ago by
I'm not so sure about UI issues here and elsewhere, either. I'm much in favor of those tickets proposing functions with keywords handling this, like rewrite
or expand(rules="sum")
. Please see wiki:symbolics#simplifyexpandsubstituteandrelationequalitytickets for these.
If we stick to expand
and rewrite
, what about expand_sum
?
comment:13 Changed 5 years ago by
I like expand_sum
, and it would presumably work with hold=True
later as well.
I'm much in favor of those tickets proposing functions with keywords handling this,
In retrospect that would have been a good design discussion to have had. Though I should say that with tabcompletion, I like the current system better. Maxima has all these flags and such, and you have to know about them. Your time is better spent adding the functionality you are  and I know it isn't getting reviewed very quickly, but experience says that they will get reviewed! A number of the people who were pretty involved in symbolics have different jobs or school now, so they haven't been able to be as involved. (I would have liked Burcin's input on this naming scheme, for instance, so it's not just two people.) Pros and cons of open source development...
comment:14 Changed 5 years ago by
 Branch changed from u/rws/symbolic_sums_should_evaluate to u/rws/15346
comment:15 Changed 5 years ago by
 Commit changed from 37f26afb68fdf136b9568db127395f6115d22a92 to 17dda26e29b6f381ee8eab545923a55e3bc9b4e6
comment:16 Changed 5 years ago by
Apply ``simplify_factorial``, ``simplify_trig``, ``simplify_rational``, ``simplify_log``, again ``simplify_rational``, and then ``expand_sum`` to self (in that order).
but
x = self x = x.simplify_factorial() x = x.simplify_rectform() x = x.simplify_trig() x = x.simplify_rational() x = x.simplify_log('one') x = x.simplify_rational() x = x.expand_sum() return x
comment:17 Changed 5 years ago by
 Commit changed from 17dda26e29b6f381ee8eab545923a55e3bc9b4e6 to cb78603878bc1a7443e982824e13c81e22e9edda
Branch pushed to git repo; I updated commit sha1. New commits:
cb78603  15346: cosmetics

comment:18 followup: ↓ 19 Changed 5 years ago by
 Cc mjo added
 Reviewers set to KarlDieter Crisman
As I suspected, the addition of the log thing led to some issues. Is this 'simplification' always valid for complex numbers? I know mjo has been very good at thinking about this kind of thing, cc:ing him.
File "src/sage/symbolic/expression.pyx", line 8249, in sage.symbolic.expression.Expression.simplify_full Failed example: f.simplify_full() Expected: log(x*y)  log(x)  log(y) Got: 0 ********************************************************************** File "src/sage/modules/vector_symbolic_dense.py", line 19, in sage.modules.vector_symbolic_dense Failed example: u.simplify_full() Expected: (1, log(3*y) + log(2*y)) Got: (1, log(6*y^2)) **********************************************************************
Naturally, this is orthogonal to the main issue, but I think it should be in a different ticket unless it's very obvious it's okay.
As to the main question, I think my only question is whether this should definitely be incorporated in simplify_full
. Can anyone think of a reason not to do so? I'm straining my brain to be creative here  for instance, if it should make a really really really long expression which isn't "simpler"? I can't find it but there was some Maxima comment about the massive length of some such expressions. My druthers would be to keep this separate from simplify_full
for now, though I understand the reasons for doing it!
comment:19 in reply to: ↑ 18 Changed 5 years ago by
Replying to kcrisman:
As I suspected, the addition of the log thing led to some issues.
This is just a conflict from having two branches modify simplify_full
at the same time. You can rebase this one on top of "develop" now and I think it'll be fine. To avoid conflicts, I would,
 Copy/paste the
expand_sum
method somewhere  reset hard your branch to match "develop"
 Add back the
expand_sum
method  Update
simplify_full
and its docstring  Recommit
I'm sure there's a smarter way to do the copy/paste step, but it works.
Is this 'simplification' always valid for complex numbers? I know mjo has been very good at thinking about this kind of thing, cc:ing him.
In the examples, n
and k
are complex, and it doesn't make any sense to sum from, say, zero a complex number. But if anything, the bug there is that sum()
allows you to give it noninteger limits.
The Maxima docs aren't too clear about what simplify_sum
does, so it's hard to say whether or not it's safe when the summand contains complex variables. We can try it and see what happens. Since it's hidden beneath simplify_full
we could always go back and remove it if it causes problems.
comment:20 followup: ↓ 21 Changed 5 years ago by
Is this 'simplification' always valid for complex numbers? I know mjo has been very good at thinking about this kind of thing, cc:ing him.
Sorry, I meant the log bit. Wait, was that already in develop? I merged develop into this branch locally, should have been uptodate.
I'm not worried about the summation with complex variables, for the record, just trying to brainstorm.
comment:21 in reply to: ↑ 20 Changed 5 years ago by
comment:22 followups: ↓ 23 ↓ 26 Changed 5 years ago by
 Dependencies set to #17731
Okay, so let's make that a dependency for this so that rws can rebase to that. Sorry, Ralf, that's not your fault  good thing I ran all the tests! And I thought I had recalled removing that simplification from simplify_full
.
comment:23 in reply to: ↑ 22 Changed 5 years ago by
Replying to kcrisman:
Okay, so let's make that a dependency for this so that rws can rebase to that. Sorry, Ralf, that's not your fault  good thing I ran all the tests! And I thought I had recalled removing that simplification from
simplify_full
.
I get more confused the more I look at this. Ralf's branch in ticket #17556 had mine as a parent, so my commits should have gone in along with his. After switching my branches around a few times, I see them now in develop.
If you git checkout develop
and then git pull
, do you see them there? (You can do git log
then hit forwardslash to search.) If they were there to begin with, there should have been a merge conflict with this branch. But those two simplify doctests shouldn't be failing unless you wound up with a chimera.
comment:24 Changed 5 years ago by
If you git checkout develop and then git pull, do you see them there?
Yes. Though I usually pull from trac not origin. But in any case the branch I was working with for this ticket also has that already in it. Yet on that branch I get
sage: z = log(3*x)+log(2*x) sage: z.simplify_full() log(6*x^2)
I don't understand either. And
$ git merge develop Already uptodate.
as expected.
comment:25 Changed 5 years ago by
Ok, I think I see what happened. False alarm. Looking at Ralf's original commit in comment 6, he added simplify_sum
to simplify_full
, but that was before I removed simplify_log
and the redundant simplify_rational
.
In comment 10, the git bot merges with develop, pulling in the patch that removed simplify_log
and the redundant simplify_rational
. But then in the commit from comment 16, the simplify_log
and simplify_rational
are back and git thinks they're being readded.
I'm not sure exactly where things went awry. At this point I usually throw my hands up, git reset hard
, and then copy/paste paste the code back where it's supposed to go. You can forcepush the branch and check the link above to make sure it's not readding simplify_log
.
comment:26 in reply to: ↑ 22 Changed 5 years ago by
Replying to kcrisman:
Sorry, Ralf, that's not your fault
Disagree. I remember too that at some time I removed it before doing a local commit. But in 17dda26 I seem to have added the log involuntarily nevertheless. That's why it's in this ticket branch and I'll remove it now. Then Michael seemed to assume that a different cause happened and based #17731 on that. So it's really me that has to apologize.
comment:27 Changed 5 years ago by
 Commit changed from cb78603878bc1a7443e982824e13c81e22e9edda to c2de94ad4765dfd62bb9c779dec9c0b387b7f949
Branch pushed to git repo; I updated commit sha1. New commits:
c2de94a  15346: sync to develop

comment:28 Changed 5 years ago by
Okay, I got you on that. I'll rerun tests this morning and add my stashed example I was going to add as a reviewer patch by the end of the day.
comment:29 Changed 5 years ago by
 Dependencies #17731 deleted
comment:30 Changed 5 years ago by
 Branch changed from u/rws/15346 to u/kcrisman/15346
 Commit changed from c2de94ad4765dfd62bb9c779dec9c0b387b7f949 to 3ee8c6d29ce916b70e5c0f32a0ae961796344afd
Okay. So I am happy with this, modulo the question about whether this really belongs in simplify_full
 again, just because it could make something VERY long and not at all "simple". I assume Ralf and I differ on this. Michael, would you like to be the tiebreak? I don't want to hold up this wrapping of Maxima's functionality.
New commits:
3ee8c6d  Additional example

comment:31 Changed 5 years ago by
I think it's OK to have it in simplify_full
. It's really annoying to have a sum of integers like,
sage: f(n=8) sum(abs(k^2 + 8), k, 1, 8)
with no way to get it down to a single number. (You can copy/paste the sum back in, but that may not be an option.) It's also a lot to ask of our users to know every method on the Expression class, but everyone knows simplify_full
.
If it causes problems, we can remove it. Or if it makes some expressions a lot uglier, there's the hack I used in simplify_rectform
:
if complexity_measure(simplified_expr) < complexity_measure(self): return simplified_expr else: return self
I think it will be fine though.
comment:32 Changed 5 years ago by
 Status changed from needs_review to positive_review
If it causes problems, we can remove it.
Somewhat of an API change, of course. My guess is it won't come up often enough, but anyway thanks for the input!
comment:33 Changed 5 years ago by
 Milestone changed from sage6.4 to sage6.5
comment:34 Changed 5 years ago by
 Branch changed from u/kcrisman/15346 to 3ee8c6d29ce916b70e5c0f32a0ae961796344afd
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
15346: implement simplify_sum and call it from full_simplify