Partial sums are off for Fourier series of piecewise functions

[novoselt]

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)

[kcrisman]

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)

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

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

[wdj]

one line change in docstring

[wdj]

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})],

[kcrisman]

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.

[burcin]

Replying to kcrisman:

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.

[novoselt]

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?

[kcrisman]

Replying to novoselt:

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.

[mkoeppe]

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

[egourgoulhon]

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)

[kcrisman]

Excellent. Is this documented via a test?

[egourgoulhon]

Replying to kcrisman:

Excellent. Is this documented via a test?

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

[kcrisman]

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.

[egourgoulhon]

Replying to kcrisman:

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

[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!

[zimmerma]

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

[egourgoulhon]

Replying to zimmerma:

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.

[kcrisman]

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?

[egourgoulhon]

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

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

[kcrisman]

Replying to egourgoulhon:

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