Opened 9 years ago
Closed 9 years ago
#13836 closed defect (fixed)
Fix variable dependence in PiecewisePolynomial
Reported by: | christiankuper | Owned by: | burcin |
---|---|---|---|
Priority: | major | Milestone: | sage-5.10 |
Component: | symbolics | Keywords: | Piecewise, critical_points |
Cc: | wdj, pbutler, kcrisman | Merged in: | sage-5.10.beta4 |
Authors: | Andrew Fleckenstein | Reviewers: | Burcin Erocal |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
When using the example in the documentation of the function the following results are displayed for critical points:
sage: R.<x> = QQ[] sage: f1 = x^0 sage: f2 = 10*x - x^2 sage: f3 = 3*x^4 - 156*x^3 + 3036*x^2 - 26208*x sage: f = Piecewise([[(0,3),f1],[(3,10),f2],[(10,20),f3]]) sage: f.critical_points() [5.0, 12.000000000000124, 12.999999999999725, 14.000000000000151]
When doing the same with y instead of x as a variable an empty list is returned as a result:
sage: R.<y> = QQ[] sage: f1 = y^0 sage: f2 = 10*y - y^2 sage: f3 = 3*y^4 - 156*y^3 + 3036*y^2 - 26208*y sage: f = Piecewise([[(0,3),f1],[(3,10),f2],[(10,20),f3]]) sage: f.critical_points() []
This behavior does not change even if y is explicitly defined as the variable:
sage: f = Piecewise([[(0,3),f1],[(3,10),f2],[(10,20),f3]],y) sage: f.critical_points() []
A variable besides default_variable() is also used (to varying effects) in the functions trapezoid(), derivative(), tangent_line() and convolution(). The fourier series functions that find coefficients use a variable other than default_variable() in the code, but it doesn't affect output, and probably doesn't need to be changed. However, the _fourier_series_helper() function probably should be changed.
sage: y = var('y') sage: f(y) = y^2 sage: f = Piecewise([[(-1, 1),f]]) sage: f._fourier_series_helper(3, 1, lambda n: 1) -4*cos(pi*x)/pi^2 + cos(2*pi*x)/pi^2 + 1/3
It should be possible to obtain consistent results no matter what the name of the variable is.
Apply :
Attachments (5)
Change History (43)
Changed 9 years ago by
comment:1 Changed 9 years ago by
comment:2 Changed 9 years ago by
- Status changed from new to needs_review
comment:3 follow-up: ↓ 4 Changed 9 years ago by
- Reviewers set to Burcin Erocal
- Status changed from needs_review to needs_work
Thanks for the quick patch. Can you add doctests to demonstrate the fix? The example in the description should be enough.
It would be good to make sure this plays well with the var
argument of the PiecewisePolynomial
constructor and the default_variable()
function. I don't think this is necessary for a positive review though.
Changed 9 years ago by
comment:4 in reply to: ↑ 3 Changed 9 years ago by
Replying to burcin:
Thanks for the quick patch. Can you add doctests to demonstrate the fix? The example in the description should be enough.
It would be good to make sure this plays well with the
var
argument of thePiecewisePolynomial
constructor and thedefault_variable()
function. I don't think this is necessary for a positive review though.
I put in the doctests in the ...withdoc.patch, as well as a testing of the var
argument and the default_variable()
function, it worked surprisingly well :-D
comment:5 Changed 9 years ago by
- Status changed from needs_work to needs_review
comment:6 follow-up: ↓ 9 Changed 9 years ago by
Hello Andrew,
thank you for the patch. It indeed seems to solve the reported problem. However, allow me to make a few remarks:
- I am not sure whether you intentionally put the patch and the doctest for the patch in two seperate patches
- I think the second part of your doctest does not proove anything as y is the only variable in your PiecewisePolynomial?. I can do the following:
sage: f1(y) = y^0 sage: f2(y) = 10*y - y^2 sage: f3(y) = 3*y^4 - 156*y^3 + 3036*y^2 - 26208*y sage: f = Piecewise([[(0,3),f1],[(3,10),f2],[(10,20),f3]], y) sage: f.critical_points() [5.0, 12.000000000000124, 12.999999999999725, 14.000000000000151] sage: z = var("z") sage: g = Piecewise([[(0,3),f1],[(3,10),f2],[(10,20),f3]], z) sage: g.critical_points() [5.0, 12.000000000000124, 12.999999999999725, 14.000000000000151]
- From personal experience: I would avoid doctests with precion outputs like this because they might fail due to slightly different results on 32 bit machines.
- The following modification results in an error, although e is a constant and not a variable. However, I think this is (also) a problem related to maxima. At least the exception raised currently is misleading. I modified the f2 in the example above:
sage: f2(y) = e^2*y - y^2 sage: f = Piecewise([[(0,3),f1],[(3,10),f2],[(10,20),f3]], y) sage: f.critical_points() ... ValueError: No differentiation variable specified.
- I looked through the piecewise.py module to see whether there are other instances of forcing the variable to x. There are indeed, but the errors they are causing are different (see example below). Do you think it would be sensible to deal with these cases also in this patch (as they are related to the same root cause)? Here is an example were the output is basically correct but the variable is wrong (g is the function defined above):
sage: g.derivative() Piecewise defined function with 3 parts, [[(0, 3), x |--> 0], [(3, 10), x |--> 0], [(10, 20), x |--> 0]]
Christian
comment:7 Changed 9 years ago by
- Status changed from needs_review to needs_work
comment:8 Changed 9 years ago by
- Description modified (diff)
- Summary changed from PiecewisePolynomial.critical_points() results depend on variable of function to Fix variable dependence in PiecewisePolynomial
comment:9 in reply to: ↑ 6 Changed 9 years ago by
- Description modified (diff)
I was going to change laplace too, but after looking at it more closely I decided to leave it alone. I had no idea what to do with convolution().
Replying to christiankuper:
- I am not sure whether you intentionally put the patch and the doctest for the patch in two seperate patches
No, it was a mistake, sorry.
- I think the second part of your doctest does not proove anything as y is the only variable in your PiecewisePolynomial?.
I put this in the doctest because Burcin suggested I made sure that default_variable() worked well, but it does seem a little redundant.
- From personal experience: I would avoid doctests with precion outputs like this because they might fail due to slightly different results on 32 bit machines.
Fixed this.
- The following modification results in an error, although e is a constant and not a variable. However, I think this is (also) a problem related to maxima. At least the exception raised currently is misleading. I modified the f2 in the example above:
sage: f2(y) = e^2*y - y^2 sage: f = Piecewise([[(0,3),f1],[(3,10),f2],[(10,20),f3]], y) sage: f.critical_points() ... ValueError: No differentiation variable specified.
This also fails if you try to do it in the current version of SAGE (with x as a variable). So yes, it is a bug in maxima. Also, I get the error
TypeError: ECL says: Error executing code in Maxima: allroots: expected a polynomial in one variable; found variables [%e,x]
instead of a ValueError?.
- I looked through the piecewise.py module to see whether there are other instances of forcing the variable to x. There are indeed, but the errors they are causing are different (see example below). Do you think it would be sensible to deal with these cases also in this patch (as they are related to the same root cause)? Here is an example were the output is basically correct but the variable is wrong (g is the function defined above):
sage: g.derivative() Piecewise defined function with 3 parts, [[(0, 3), x |--> 0], [(3, 10), x |--> 0], [(10, 20), x |--> 0]]
Changed the description to reflect this.
comment:10 Changed 9 years ago by
- Status changed from needs_work to needs_review
comment:11 follow-up: ↓ 13 Changed 9 years ago by
Could you please
- fold the patches into one patch
- replace everywhere the sentence "trac #13836" by the automatic link
:trac:`13836`
comment:12 Changed 9 years ago by
- Cc kcrisman added
comment:13 in reply to: ↑ 11 ; follow-up: ↓ 14 Changed 9 years ago by
comment:14 in reply to: ↑ 13 Changed 9 years ago by
- replace everywhere the sentence "trac #13836" by the automatic link :trac:
13836
I'm not sure where I need to do this. Do you mean in the doctests?
Correct, in attachment:trac_13836_piecewise_critpoints_withdoc.patch and attachment:trac_13836_piecewise_var_fix.patch there are three references to #13836
.
Or is only attachment:trac_13836_piecewise_var_fix.patch needed? You should make that clear in the ticket Description.
comment:15 Changed 9 years ago by
- Description modified (diff)
comment:16 Changed 9 years ago by
Sorry, the syntax of the :trac:
is not quite correct in the latest patch. It should contain `
symbols, namely :trac:`13836`
You should check that it gives a good result when asking for the doc in the notebook.
comment:17 follow-up: ↓ 18 Changed 9 years ago by
- Status changed from needs_review to needs_work
Hello Andrew,
I think there is still a small inconsistency in your code: In most methods you use the default_variable as the parameter. However, in critical_points() you just deleted old asignment of using "x" as the variable. While the other methods work with expressions with more than variable, critical_points() raises a TypeError?.
What would be even nicer than using the default_variable IMHO would be the use of the "var" parameter in the constructor (if defined). What do yout think? Or maybe this would be something for a nother patch.
Christian
comment:18 in reply to: ↑ 17 Changed 9 years ago by
Replying to christiankuper:
Hello Andrew,
I think there is still a small inconsistency in your code: In most methods you use the default_variable as the parameter. However, in critical_points() you just deleted old asignment of using "x" as the variable. While the other methods work with expressions with more than variable, critical_points() raises a TypeError?.
What would be even nicer than using the default_variable IMHO would be the use of the "var" parameter in the constructor (if defined). What do yout think? Or maybe this would be something for a nother patch.
Christian
I fixed the critical_points inconsistency, thank you for pointing that out. As for the default_variable(), I was just using what was already written for the purpose, I think it's rather helpful. If var is undefined, we have to end up using default_variable() anyway, so IMHO it would be better to keep it a little simpler.
Andrew
P.S., I fixed a small grammatical error in the docstring for default_variable().
comment:19 Changed 9 years ago by
- Status changed from needs_work to needs_review
comment:20 follow-up: ↓ 22 Changed 9 years ago by
Hello Andrew,
sorry for the long silence but now finally I find time to look into your patch. Running the doctest I currently get one failure:
********************************************************************** File "/opt/sage-dev3/devel/sage-main/sage/functions/piecewise.py", line 501: sage: f.trapezoid(4) Expected: Piecewise defined function with 4 parts, [[(0, 1/2), 1/2*y], [(1/2, 1), 9/2*y - 2], [(1, 3/2), 1/2*y + 2], [(3/2, 2), -7/2*y + 8]] Got: Piecewise defined function with 4 parts, [[(0, 1/2), 1/2*x], [(1/2, 1), 9/2*x - 2], [(1, 3/2), 1/2*x + 2], [(3/2, 2), -7/2*x + 8]] **********************************************************************
I am not sure why this is happening but will try to look into it.
Christian
comment:21 Changed 9 years ago by
- Status changed from needs_review to needs_work
comment:22 in reply to: ↑ 20 ; follow-up: ↓ 23 Changed 9 years ago by
Replying to christiankuper:
Running the doctest I currently get one failure:
That's weird, because when I test piecewise.py everything passes:
andrew@andrew-laptop ~/sage-5.6/devel/sage $ ../../sage -t --long sage/functions/piecewise.py sage -t --long "devel/sage-main/sage/functions/piecewise.py" [26.7 s] ---------------------------------------------------------------------- All tests passed! Total time for all tests: 26.7 seconds
I also manually tested the code, (which I assumed to be my addition to the trapezoid docstring) and achieved the expected results.
comment:23 in reply to: ↑ 22 Changed 9 years ago by
Hmm, that really is weird. Hopefully I loaded the correct patch (trac_13836_piecewise_var_fix.patch?).
comment:24 follow-up: ↓ 25 Changed 9 years ago by
Hello Andrew,
finally found the time to study this patch again. Meanwhile I updated to 5.7 and could not reproduce the error I reported. However, two new ones popped up, although the second one IMHO is just related to me not having build the complete doc.
The following tests failed: sage -t --long -force_lib "devel/sage/doc/en/constructions/calculus.rst" sage -t --long -force_lib "devel/sage/sage/misc/sagedoc.py"
The details from the log file:
sage -t --long -force_lib "devel/sage/doc/en/constructions/calculus.rst" ********************************************************************** File "/opt/sage-dev3/devel/sage/doc/en/constructions/calculus.rst", line 377: sage: f.fourier_series_partial_sum(3,pi) Exception raised: Traceback (most recent call last): File "/opt/sage-dev3/local/bin/ncadoctest.py", line 1231, in run_one_test self.run_one_example(test, example, filename, compileflags) File "/opt/sage-dev3/local/bin/sagedoctest.py", line 38, in run_one_example OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags) File "/opt/sage-dev3/local/bin/ncadoctest.py", line 1172, in run_one_example compileflags, 1) in test.globs File "<doctest __main__.example_16[7]>", line 1, in <module> f.fourier_series_partial_sum(Integer(3),pi)###line 377: sage: f.fourier_series_partial_sum(3,pi) File "/opt/sage-dev3/local/lib/python/site-packages/sage/functions/piecewise.py", line 1192, in fourier_series_partial_sum return self._fourier_series_helper(N, L, lambda n: 1) File "/opt/sage-dev3/local/lib/python/site-packages/sage/functions/piecewise.py", line 1161, in _fourier_series_helper x = self.default_variable() File "/opt/sage-dev3/local/lib/python/site-packages/sage/functions/piecewise.py", line 719, in default_variable fun = SR(fun) File "parent.pyx", line 834, in sage.structure.parent.Parent.__call__ (sage/structure/parent.c:7415) File "coerce_maps.pyx", line 82, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (sage/structure/coerce_maps.c:3583) File "coerce_maps.pyx", line 77, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (sage/structure/coerce_maps.c:3485) File "ring.pyx", line 284, in sage.symbolic.ring.SymbolicRing._element_constructor_ (sage/symbolic/ring.cpp:4879) TypeError
I believe that the error is caused by the fact that piecewise.default_variable() raises a TypeError? when lambda functions are used (the example in calculus.rst uses lambda functions). Correct me if I am worng but I don't think this is related to anything you changed in piecewise.
So I do not know whether this patch can be set to positive review or whether that bug needs to be fixed first.
Christian
comment:25 in reply to: ↑ 24 Changed 9 years ago by
Replying to christiankuper:
Correct me if I am worng but I don't think this is related to anything you changed in piecewise.
I don't know any better than you do, perhaps we could get someone else's opinion?
comment:26 Changed 9 years ago by
Just spent a few hours testing and looking into this patch. Forget what I said before: This error IS caused by this patch. After applying the patch piecewise.py cannot handle lambda functions any more.
Christian
comment:27 Changed 9 years ago by
- Description modified (diff)
comment:28 follow-up: ↓ 29 Changed 9 years ago by
Hello Andrew,
I added a little piece of code causing piecewise.default_variable() to default to 'x' if lambda functions are used. This removes the doctest errors. Waht do you think?
Christian
comment:29 in reply to: ↑ 28 ; follow-up: ↓ 30 Changed 9 years ago by
Replying to christiankuper:
Hello Andrew,
I added a little piece of code causing piecewise.default_variable() to default to 'x' if lambda functions are used. This removes the doctest errors. Waht do you think?
Christian
Looks good to me! Thanks for figuring it out, I was kind of lost at this part.
Andrew
comment:30 in reply to: ↑ 29 Changed 9 years ago by
Looks good to me! Thanks for figuring it out, I was kind of lost at this part.
Andrew
I would suggest you put the ticket to 'needs_review' then.
comment:31 Changed 9 years ago by
- Status changed from needs_work to needs_review
comment:32 Changed 9 years ago by
- Status changed from needs_review to positive_review
As I could find no more bugs I will put the patch on positive_review. Thanks for the fix.
Christian
comment:33 Changed 9 years ago by
trac_13836-rev.patch needs a proper commit message.
comment:34 Changed 9 years ago by
- Status changed from positive_review to needs_work
The documentation doesn't build properly:
dochtml.log:[functions] /mazur/release/merger/sage-5.9.rc0/local/lib/python2.7/site-packages/sage/functions/piecewise.py:docstring of sage.functions.piecewise.PiecewisePolynomial.critical_points:23: WARNING: Literal block expected; none found. dochtml.log:[functions] /mazur/release/merger/sage-5.9.rc0/local/lib/python2.7/site-packages/sage/functions/piecewise.py:docstring of sage.functions.piecewise.PiecewisePolynomial.trapezoid:36: WARNING: Literal block expected; none found.
Changed 9 years ago by
Changed 9 years ago by
comment:35 Changed 9 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
comment:36 Changed 9 years ago by
- Description modified (diff)
comment:37 Changed 9 years ago by
- Status changed from needs_review to positive_review
comment:38 Changed 9 years ago by
- Merged in set to sage-5.10.beta4
- Resolution set to fixed
- Status changed from positive_review to closed
Yes, the code practically forced x to be the variable. It should be fine now, the fix was simple.