Opened 9 years ago

Closed 18 months ago

Last modified 18 months ago

#8603 closed defect (duplicate)

Partial sums are off for Fourier series of piecewise functions

Reported by: novoselt Owned by: burcin
Priority: minor Milestone: sage-duplicate/invalid/wontfix
Component: calculus Keywords: sd31
Cc: wdj, jason, jondo, kcrisman, vbraun, slelievre, mkoeppe, eviatarbach, rws, novoselt Merged in:
Authors: David Joyner Reviewers:
Report Upstream: N/A Work issues: other instances of the typo
Branch: Commit:
Dependencies: #14801 Stopgaps:

Description (last modified by egourgoulhon)

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)

Attachments (1)

trac_8603-fourier-sum-docstring.patch (957 bytes) - added by wdj 8 years ago.
one line change in docstring

Download all attachments as: .zip

Change History (29)

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

  • Keywords sd31 added

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

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.

comment:7 follow-up: 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

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.

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 21 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: Changed 21 months ago by kcrisman

Excellent. Is this documented via a test?

comment:17 Changed 21 months ago by kcrisman

  • Status changed from positive_review to needs_info

comment:18 in reply to: ↑ 16 Changed 21 months ago by 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.

comment:19 follow-up: Changed 21 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 21 months ago by 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))

comment:21 Changed 21 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 18 months ago by embray

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

comment:23 follow-up: Changed 18 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 18 months ago by 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.

comment:25 Changed 18 months ago by egourgoulhon

  • Description modified (diff)

comment:26 follow-up: Changed 18 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: Changed 18 months ago by 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).

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

comment:28 in reply to: ↑ 27 Changed 18 months ago by 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.

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