Opened 9 years ago

Closed 9 years ago

# Fix variable dependence in PiecewisePolynomial

Reported by: Owned by: christiankuper burcin major sage-5.10 symbolics Piecewise, critical_points wdj, pbutler, kcrisman sage-5.10.beta4 Andrew Fleckenstein Burcin Erocal N/A

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 :

### comment:1 Changed 9 years ago by afleckenstein

Yes, the code practically forced x to be the variable. It should be fine now, the fix was simple.

### comment:2 Changed 9 years ago by afleckenstein

• Status changed from new to needs_review

### comment:3 follow-up: ↓ 4 Changed 9 years ago by burcin

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

### comment:4 in reply to: ↑ 3 Changed 9 years ago by afleckenstein

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.

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 afleckenstein

• Authors changed from Christian Kuper to Andrew Fleckenstein
• Status changed from needs_work to needs_review

### comment:6 follow-up: ↓ 9 Changed 9 years ago by christiankuper

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 christiankuper

• Status changed from needs_review to needs_work

### comment:8 Changed 9 years ago by afleckenstein

• 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 afleckenstein

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

• 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 afleckenstein

• Status changed from needs_work to needs_review

### comment:11 follow-up: ↓ 13 Changed 9 years ago by chapoton

• fold the patches into one patch
• replace everywhere the sentence "trac #13836" by the automatic link `:trac:`13836``
Last edited 9 years ago by chapoton (previous) (diff)

### comment:12 Changed 9 years ago by kcrisman

• Cc kcrisman added

### comment:13 in reply to: ↑ 11 ; follow-up: ↓ 14 Changed 9 years ago by afleckenstein

• 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?

### comment:14 in reply to: ↑ 13 Changed 9 years ago by kcrisman

• 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 afleckenstein

• Description modified (diff)

### comment:16 Changed 9 years ago by chapoton

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 christiankuper

• 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 afleckenstein

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 afleckenstein

• Status changed from needs_work to needs_review

### comment:20 follow-up: ↓ 22 Changed 9 years ago by christiankuper

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 christiankuper

• Status changed from needs_review to needs_work

### comment:22 in reply to: ↑ 20 ; follow-up: ↓ 23 Changed 9 years ago by afleckenstein

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 christiankuper

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 christiankuper

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 afleckenstein

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 christiankuper

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

### Changed 9 years ago by christiankuper

Catch errors due to lambda functions

### comment:27 Changed 9 years ago by christiankuper

• Description modified (diff)

### comment:28 follow-up: ↓ 29 Changed 9 years ago by 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

### comment:29 in reply to: ↑ 28 ; follow-up: ↓ 30 Changed 9 years ago by afleckenstein

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 christiankuper

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 afleckenstein

• Status changed from needs_work to needs_review

### comment:32 Changed 9 years ago by christiankuper

• 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 jdemeyer

trac_13836-rev.patch needs a proper commit message.

### comment:34 Changed 9 years ago by jdemeyer

• 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.
```

### comment:35 Changed 9 years ago by afleckenstein

• Description modified (diff)
• Status changed from needs_work to needs_review

### comment:36 Changed 9 years ago by afleckenstein

• Description modified (diff)

### comment:37 Changed 9 years ago by afleckenstein

• Status changed from needs_review to positive_review

### comment:38 Changed 9 years ago by jdemeyer

• Merged in set to sage-5.10.beta4
• Resolution set to fixed
• Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.