#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 )
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)
Change History (29)
comment:1 Changed 11 years ago by
- Priority changed from major to minor
comment:2 Changed 11 years ago by
- 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 11 years ago by
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.
comment:4 Changed 11 years ago by
- 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 11 years ago by
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 11 years ago by
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: ↓ 8 Changed 11 years ago by
- 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 11 years ago by
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 9 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:10 Changed 8 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:11 Changed 8 years ago by
- Dependencies set to #14801
comment:12 Changed 8 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:13 Changed 8 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:14 Changed 6 years ago by
- 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 5 years ago by
- 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 5 years ago by
Excellent. Is this documented via a test?
comment:17 Changed 5 years ago by
- Status changed from positive_review to needs_info
comment:18 in reply to: ↑ 16 Changed 5 years ago by
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: ↓ 20 Changed 5 years ago by
- 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 5 years ago by
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 5 years ago by
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 5 years ago by
- Resolution set to wontfix
- Status changed from positive_review to closed
comment:23 follow-up: ↓ 24 Changed 5 years ago by
I wonder why "wontfix" since the issue is fixed in 8.1.beta4. Paul
comment:24 in reply to: ↑ 23 Changed 5 years ago by
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 5 years ago by
- Description modified (diff)
comment:26 follow-up: ↓ 27 Changed 5 years ago by
- 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 5 years ago by
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).
comment:28 in reply to: ↑ 27 Changed 5 years ago by
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.
This is still true, and syntax is also deprecated.