Opened 9 years ago

Closed 15 months ago

# Partial sums are off for Fourier series of piecewise functions

Reported by: Owned by: novoselt burcin minor sage-duplicate/invalid/wontfix calculus sd31 wdj, jason, jondo, kcrisman, vbraun, slelievre, mkoeppe, eviatarbach, rws, novoselt David Joyner N/A other instances of the typo #14801

Doing

f = Piecewise([[(-pi, pi), x]])
print f.fourier_series_partial_sum(2, pi)
print f.fourier_series_partial_sum(3, pi)


we get

2*sin(x)
-sin(2*x) + 2*sin(x)


while according to the documentation we should get the second output with the first command.

Update: Same output with the new piecewise from #14801. Does it agree with the documentation there?

UPDATE: this is fixed in Sage 8.1 (see #23672):

sage: f = piecewise([[(-pi, pi), x]])
sage: f.fourier_series_partial_sum(2, pi)
-sin(2*x) + 2*sin(x)


### comment:1 Changed 8 years ago by kcrisman

• Priority changed from major to minor

This is still true, and syntax is also deprecated.

sage: f = Piecewise([[(-pi, pi), x]])
sage: print f.fourier_series_partial_sum(2, pi)
/Applications/MathApps/sage/local/lib/python2.6/site-packages/sage/functions/piecewise.py:1095: DeprecationWarning: Substitution using function-call syntax and unnamed arguments is deprecated and will be removed from a future release of Sage; you can use named arguments instead, like EXPR(x=..., y=...)
a0 = self.fourier_series_cosine_coefficient(0,L)
/Applications/MathApps/sage/local/lib/python2.6/site-packages/sage/functions/piecewise.py:1099: DeprecationWarning: Substitution using function-call syntax and unnamed arguments is deprecated and will be removed from a future release of Sage; you can use named arguments instead, like EXPR(x=..., y=...)
for n in srange(1,N)])
2*sin(x)
sage: print f.fourier_series_partial_sum(3, pi)
-sin(2*x) + 2*sin(x)


### comment:2 Changed 8 years ago by novoselt

On a related note: what is the purpose of plot methods for Fourier partial sums? They don't do anything except for passing arguments to the usual global plot function, so they seem redundant to me and perhaps can be removed (after deprecation period).

### comment:3 Changed 8 years ago by kcrisman

You may be right. I'd have to look at it. Remember, these are all really old, so they probably at the time bypassed the non-existent 'plot' function, and then were subsequently changed, perhaps.

### Changed 8 years ago by wdj

one line change in docstring

### comment:4 Changed 8 years ago by wdj

• Status changed from new to needs_review

This fixes the documentation of fourier_series_partial_sum, replacing

 f(x) \sim \frac{a_0}{2} + \sum_{n=1}^N [a_n\cos(\frac{n\pi x}{L}) +
b_n\sin(\frac{n\pi x}{L})],


by

 f(x) \sim \frac{a_0}{2} + \sum_{n=1}^{N-1} [a_n\cos(\frac{n\pi x}{L})
+ b_n\sin(\frac{n\pi x}{L})],


### comment:5 follow-up: ↓ 6 Changed 8 years ago by kcrisman

• Authors set to David Joyner

Thanks, David; I don't have time to review this now, but appreciate it.

Andrey and I were discussing this at Sage Days 31, and thought that maybe changing the behavior instead to match Taylor series would be good, but if this was in fact what you had intended all along, then this solution is better.

### comment:6 in reply to: ↑ 5 Changed 8 years ago by burcin

Thanks, David; I don't have time to review this now, but appreciate it.

Andrey and I were discussing this at Sage Days 31, and thought that maybe changing the behavior instead to match Taylor series would be good, but if this was in fact what you had intended all along, then this solution is better.

The .series() method of symbolic expressions cut off in a pythonic way, like David's change here. If .taylor() does something different we should change it.

This is a trivial change. I'd be happy to give a positive review if it passes all tests but the patch bot doesn't seem to be working for some reason.

### comment:7 follow-up: ↓ 8 Changed 8 years ago by novoselt

• Status changed from needs_review to needs_work
• Work issues set to other instances of the typo

There are more instances of the same typo in other functions of this module, let's fix them all at once!-)

David, do you agree that plot methods can be eliminated as they are not really doing anything?

### comment:8 in reply to: ↑ 7 Changed 8 years ago by kcrisman

There are more instances of the same typo in other functions of this module, let's fix them all at once!-)

Can you be more specific?

David, do you agree that plot methods can be eliminated as they are not really doing anything?

I think I looked at this at Sage Days 31, but now I forgot whether that statement is true.

### comment:9 Changed 6 years ago by jdemeyer

• Milestone changed from sage-5.11 to sage-5.12

### comment:10 Changed 5 years ago by vbraun_spam

• Milestone changed from sage-6.1 to sage-6.2

### comment:11 Changed 5 years ago by ppurka

• Dependencies set to #14801

### comment:12 Changed 5 years ago by vbraun_spam

• Milestone changed from sage-6.2 to sage-6.3

### comment:13 Changed 5 years ago by vbraun_spam

• Milestone changed from sage-6.3 to sage-6.4

### comment:14 Changed 3 years ago by mkoeppe

• Cc wdj jason jondo kcrisman vbraun slelievre mkoeppe eviatarbach rws novoselt added
• Description modified (diff)
• Milestone changed from sage-6.4 to sage-7.3
• Status changed from needs_work to needs_info

Updated with information regarding the new piecewise implementation from #14801.

### comment:15 Changed 18 months ago by egourgoulhon

• Milestone changed from sage-7.3 to sage-duplicate/invalid/wontfix
• Status changed from needs_info to positive_review

This is fixed by #23672.

Regarding the example in the ticket description, in Sage 8.1.beta4, we have now

sage: f = piecewise([[(-pi, pi), x]])
sage: f.fourier_series_partial_sum(2, pi)
-sin(2*x) + 2*sin(x)


We even have, since the half-period is now a default argument,

sage: f.fourier_series_partial_sum(2)
-sin(2*x) + 2*sin(x)


### comment:16 follow-up: ↓ 18 Changed 18 months ago by kcrisman

Excellent. Is this documented via a test?

### comment:17 Changed 18 months ago by kcrisman

• Status changed from positive_review to needs_info

### comment:18 in reply to: ↑ 16 Changed 18 months ago by egourgoulhon

Excellent. Is this documented via a test?

Yes this is documented, both in Sage Reference Manual and in Sage Constructions, see here.

### comment:19 follow-up: ↓ 20 Changed 18 months ago by kcrisman

• Status changed from needs_info to positive_review

Sweet. Strange that it didn't cause any doctest errors then? If it didn't, we should make sure to include at least two of the examples on the ticket in the doc somewhere.

### comment:20 in reply to: ↑ 19 Changed 18 months ago by egourgoulhon

Sweet. Strange that it didn't cause any doctest errors then? If it didn't, we should make sure to include at least two of the examples on the ticket in the doc somewhere.

I am not sure to understand what you mean. In the current version, as integrated in Sage 8.1.beta4, there are doctests like

sage: f = piecewise([((-1,0), -1), ((0,1), 1)])
sage: f.fourier_series_partial_sum(5)
4/5*sin(5*pi*x)/pi + 4/3*sin(3*pi*x)/pi + 4*sin(pi*x)/pi


In Sage <= 8.0, this would have returned (*)

4/3*sin(3*pi*x)/pi + 4*sin(pi*x)/pi


(*) with the half-period added as the second argument, i.e. f.fourier_series_partial_sum(5, 1))

### comment:21 Changed 18 months ago by kcrisman

My concern was just that the correct nature was doctested, not the wrong one, and that we really did have that to test against regression at some point. Good!

### comment:22 Changed 15 months ago by embray

• Resolution set to wontfix
• Status changed from positive_review to closed

### comment:23 follow-up: ↓ 24 Changed 15 months ago by zimmerma

I wonder why "wontfix" since the issue is fixed in 8.1.beta4. Paul

### comment:24 in reply to: ↑ 23 Changed 15 months ago by egourgoulhon

I wonder why "wontfix" since the issue is fixed in 8.1.beta4. Paul

Actually, as said in comment:15, the issue is fixed in another ticket: #23672, hence the "sage-duplicate/invalid/wontfix" milestone for the current one and the "wonfix" resolution.

### comment:25 Changed 15 months ago by egourgoulhon

• Description modified (diff)

### comment:26 follow-up: ↓ 27 Changed 15 months ago by kcrisman

• Resolution changed from wontfix to duplicate

Hey, do non-release managers get to mark "closed"? That would be a change in protocol.

Also, maybe the resolution should be "fixed" or "duplicate" if it is indeed fixed in another ticket?

### comment:27 in reply to: ↑ 26 ; follow-up: ↓ 28 Changed 15 months ago by egourgoulhon

Hey, do non-release managers get to mark "closed"? That would be a change in protocol.

This was announced in https://groups.google.com/d/msg/sage-release/4bIUu1NECwY/we3BMdkeAAAJ with apparently the approval of the release manager.

Also, maybe the resolution should be "fixed" or "duplicate" if it is indeed fixed in another ticket?

Ah yes, you are right (I thought this was automatically set to "wontfix" while closing "sage-duplicate/invalid/wontfix" tickets).

Last edited 15 months ago by egourgoulhon (previous) (diff)

### comment:28 in reply to: ↑ 27 Changed 15 months ago by kcrisman

Hey, do non-release managers get to mark "closed"? That would be a change in protocol.

This was announced in https://groups.google.com/d/msg/sage-release/4bIUu1NECwY/we3BMdkeAAAJ with apparently the approval of the release manager.

10 hours ago :-) but this will be welcome for obvious dupes etc.

Also, maybe the resolution should be "fixed" or "duplicate" if it is indeed fixed in another ticket?

Ah yes, you are right (I thought this was automatically set to "wontfix" while closing "sage-duplicate/invalid/wontfix" tickets).

Yeah, that might be the default, but typically we try to be precise on this. Nice.

Last edited 15 months ago by kcrisman (previous) (diff)
Note: See TracTickets for help on using tickets.