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

Symbolic sums should evaluate #15346

Closed
kcrisman opened this issue Nov 1, 2013 · 41 comments
Closed

Symbolic sums should evaluate #15346

kcrisman opened this issue Nov 1, 2013 · 41 comments

Comments

@kcrisman
Copy link
Member

kcrisman commented Nov 1, 2013

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

CC: @orlitzky

Component: symbolics

Author: Ralf Stephan

Branch/Commit: 3ee8c6d

Reviewer: Karl-Dieter Crisman

Issue created by migration from https://trac.sagemath.org/ticket/15346

@kcrisman kcrisman added this to the sage-6.1 milestone Nov 1, 2013
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.1, sage-6.2 Jan 30, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.2, sage-6.3 May 6, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.3, sage-6.4 Aug 10, 2014
@jdemeyer

This comment has been minimized.

@jdemeyer jdemeyer changed the title symbolic sums that don't simplify don't simplify Symbolic sums should evaluate Dec 2, 2014
@rwst
Copy link

rwst commented Dec 2, 2014

Branch: u/rws/symbolic_sums_should_evaluate

@rwst
Copy link

rwst commented Dec 2, 2014

New commits:

df516f815346: implement simplify_sum and call it from full_simplify

@rwst
Copy link

rwst commented Dec 2, 2014

Commit: df516f8

@rwst
Copy link

rwst commented Dec 2, 2014

Author: Ralf Stephan

@rwst
Copy link

rwst commented Dec 2, 2014

comment:7

The issue however begs the question if it should not be possible to give hold parameter to sums.

@kcrisman
Copy link
Member Author

kcrisman commented Dec 2, 2014

comment:8

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 4, 2014

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

af5657615346: add doctest

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 4, 2014

Changed commit from df516f8 to af56576

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 1, 2015

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 1, 2015

Changed commit from af56576 to 37f26af

@kcrisman
Copy link
Member Author

kcrisman commented Feb 3, 2015

comment:11

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".

@rwst
Copy link

rwst commented Feb 3, 2015

comment:12

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 symbolics#simplifyexpandsubstituteandrelationequalitytickets for these.

If we stick to expand and rewrite, what about expand_sum?

@kcrisman
Copy link
Member Author

kcrisman commented Feb 3, 2015

comment:13

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

@rwst
Copy link

rwst commented Feb 3, 2015

Changed branch from u/rws/symbolic_sums_should_evaluate to u/rws/15346

@rwst
Copy link

rwst commented Feb 3, 2015

Changed commit from 37f26af to 17dda26

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 4, 2015

Changed commit from 17dda26 to cb78603

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 4, 2015

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

cb7860315346: cosmetics

@kcrisman
Copy link
Member Author

kcrisman commented Feb 4, 2015

Reviewer: Karl-Dieter Crisman

@kcrisman
Copy link
Member Author

kcrisman commented Feb 4, 2015

comment:18

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!

@orlitzky
Copy link
Contributor

orlitzky commented Feb 4, 2015

comment:19

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.

@kcrisman
Copy link
Member Author

kcrisman commented Feb 5, 2015

comment:20

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.

@orlitzky
Copy link
Contributor

orlitzky commented Feb 5, 2015

comment:21

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.

@kcrisman
Copy link
Member Author

kcrisman commented Feb 5, 2015

Dependencies: #17731

@kcrisman
Copy link
Member Author

kcrisman commented Feb 5, 2015

comment:22

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.

@orlitzky
Copy link
Contributor

orlitzky commented Feb 5, 2015

comment:23

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.

@kcrisman
Copy link
Member Author

kcrisman commented Feb 5, 2015

comment:24

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.

@orlitzky
Copy link
Contributor

orlitzky commented Feb 5, 2015

comment:25

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.

@rwst
Copy link

rwst commented Feb 5, 2015

comment:26

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 5, 2015

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

c2de94a15346: sync to develop

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 5, 2015

Changed commit from cb78603 to c2de94a

@kcrisman
Copy link
Member Author

kcrisman commented Feb 5, 2015

comment:28

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.

@kcrisman
Copy link
Member Author

kcrisman commented Feb 5, 2015

Changed dependencies from #17731 to none

@kcrisman
Copy link
Member Author

kcrisman commented Feb 5, 2015

Changed commit from c2de94a to 3ee8c6d

@kcrisman
Copy link
Member Author

kcrisman commented Feb 5, 2015

Changed branch from u/rws/15346 to u/kcrisman/15346

@kcrisman
Copy link
Member Author

kcrisman commented Feb 5, 2015

comment:30

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

@orlitzky
Copy link
Contributor

orlitzky commented Feb 5, 2015

comment:31

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.

@kcrisman
Copy link
Member Author

kcrisman commented Feb 5, 2015

comment:32

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!

@vbraun
Copy link
Member

vbraun commented Feb 17, 2015

Changed branch from u/kcrisman/15346 to 3ee8c6d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants