Opened 11 years ago

Closed 3 years 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 10 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 10 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 10 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 10 years ago by wdj

one line change in docstring

### comment:4 Changed 10 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 10 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 10 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 10 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 10 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 8 years ago by jdemeyer

• Milestone changed from sage-5.11 to sage-5.12

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

• Milestone changed from sage-6.1 to sage-6.2

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

• Dependencies set to #14801

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

• Milestone changed from sage-6.2 to sage-6.3

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

• Milestone changed from sage-6.3 to sage-6.4

### comment:14 Changed 5 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 4 years 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 4 years ago by kcrisman

Excellent. Is this documented via a test?

### comment:17 Changed 4 years ago by kcrisman

• Status changed from positive_review to needs_info

### comment:18 in reply to: ↑ 16 Changed 4 years 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 4 years 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 4 years 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 4 years 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 3 years ago by embray

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

### comment:23 follow-up: ↓ 24 Changed 3 years ago by zimmerma

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

### comment:24 in reply to: ↑ 23 Changed 3 years 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 3 years ago by egourgoulhon

• Description modified (diff)

### comment:26 follow-up: ↓ 27 Changed 3 years 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 3 years ago by egourgoulhon

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?

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

Version 0, edited 3 years ago by egourgoulhon (next)

### comment:28 in reply to: ↑ 27 Changed 3 years 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 3 years ago by kcrisman (previous) (diff)
Note: See TracTickets for help on using tickets.