Opened 6 years ago

Closed 5 years ago

#15346 closed defect (fixed)

Symbolic sums should evaluate

Reported by: kcrisman Owned by:
Priority: major Milestone: sage-6.5
Component: symbolics Keywords:
Cc: mjo Merged in:
Authors: Ralf Stephan Reviewers: Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: 3ee8c6d (Commits) Commit: 3ee8c6d29ce916b70e5c0f32a0ae961796344afd
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

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 vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:2 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:3 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:4 Changed 5 years ago by jdemeyer

  • 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 rws

  • Branch set to u/rws/symbolic_sums_should_evaluate

comment:6 Changed 5 years ago by rws

  • Authors set to Ralf Stephan
  • Commit set to df516f88d0c13d22d60e8b7d191eb0b2aa75f26a
  • Status changed from new to needs_review

New commits:

df516f815346: implement simplify_sum and call it from full_simplify

comment:7 Changed 5 years ago by rws

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 kcrisman

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 git

  • Commit changed from df516f88d0c13d22d60e8b7d191eb0b2aa75f26a to af565761f5a154fc1745bc754c3cbbb642065baf

Branch pushed to git repo; I updated commit sha1. New commits:

af5657615346: add doctest

comment:10 Changed 5 years ago by git

  • Commit changed from af565761f5a154fc1745bc754c3cbbb642065baf to 37f26afb68fdf136b9568db127395f6115d22a92

Branch pushed to git repo; I updated commit sha1. New commits:

37f26afMerge branch 'develop' into t/15346/symbolic_sums_should_evaluate

comment:11 Changed 5 years ago by kcrisman

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 rws

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 kcrisman

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 tab-completion, 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 rws

  • Branch changed from u/rws/symbolic_sums_should_evaluate to u/rws/15346

comment:15 Changed 5 years ago by rws

  • Commit changed from 37f26afb68fdf136b9568db127395f6115d22a92 to 17dda26e29b6f381ee8eab545923a55e3bc9b4e6

Please review.


New commits:

17dda2615346: make maxima.simplify_sum available as expand_sum

comment:16 Changed 5 years ago by kcrisman

        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 git

  • Commit changed from 17dda26e29b6f381ee8eab545923a55e3bc9b4e6 to cb78603878bc1a7443e982824e13c81e22e9edda

Branch pushed to git repo; I updated commit sha1. New commits:

cb7860315346: cosmetics

comment:18 follow-up: Changed 5 years ago by kcrisman

  • Cc mjo added
  • Reviewers set to Karl-Dieter 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 mjo

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 non-integer 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 follow-up: Changed 5 years ago by kcrisman

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 up-to-date.

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 mjo

Replying to kcrisman:

Sorry, I meant the log bit. Wait, was that already in develop? I merged develop into this branch locally, should have been up-to-date.

Yikes, I guess not. I opened ticket #17731.

comment:22 follow-ups: Changed 5 years ago by kcrisman

  • 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 mjo

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 forward-slash 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 kcrisman

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 up-to-date.

as expected.

comment:25 Changed 5 years ago by mjo

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 re-added.

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 force-push the branch and check the link above to make sure it's not re-adding simplify_log.

comment:26 in reply to: ↑ 22 Changed 5 years ago by rws

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 git

  • Commit changed from cb78603878bc1a7443e982824e13c81e22e9edda to c2de94ad4765dfd62bb9c779dec9c0b387b7f949

Branch pushed to git repo; I updated commit sha1. New commits:

c2de94a15346: sync to develop

comment:28 Changed 5 years ago by kcrisman

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 kcrisman

  • Dependencies #17731 deleted

comment:30 Changed 5 years ago by kcrisman

  • 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:

3ee8c6dAdditional example

comment:31 Changed 5 years ago by mjo

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 kcrisman

  • 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 rws

  • Milestone changed from sage-6.4 to sage-6.5

comment:34 Changed 5 years ago by vbraun

  • Branch changed from u/kcrisman/15346 to 3ee8c6d29ce916b70e5c0f32a0ae961796344afd
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.