Opened 7 years ago
Last modified 4 years ago
#14801 closed defect
Piecewise functions done right — at Version 48
Reported by:  vbraun  Owned by:  rws 

Priority:  major  Milestone:  sage7.2 
Component:  symbolics  Keywords:  
Cc:  eviatarbach, kcrisman, slelievre, mkoeppe  Merged in:  
Authors:  Volker Braun, Ralf Stephan  Reviewers:  
Report Upstream:  N/A  Work issues:  
Branch:  public/piecewise2 (Commits)  Commit:  c84b28a8f3f4776dd9e994e767cddf1fc4641541 
Dependencies:  #14800, #14780, #9556, #13125, #14802, #16397, #17759  Stopgaps: 
Description (last modified by )
Rewrite piecewise functions as symbolic functions.
For a (late) discussion about interface issues see https://groups.google.com/forum/?hl=en#!topic/sagedevel/dgwUMsdiHfM
Change History (49)
comment:1 Changed 7 years ago by
comment:2 Changed 7 years ago by
 Dependencies set to #14800, #14780, #9556, #13125
comment:3 Changed 7 years ago by
 Cc eviatarbach added
comment:4 Changed 7 years ago by
 Cc kcrisman added
comment:5 Changed 6 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:6 Changed 6 years ago by
 Branch set to public/piecewise
 Commit set to 6d00b38418552984fa951bf9c4090da001a1ec9f
New commits:
6d00b38  Trac 14801: Rewrite piecewise functions as symbolic functions

comment:7 Changed 6 years ago by
Many doctests fail in the new code. For example:
sage: f = piecewise([((0,1), x^3), ([1,0], x^2)]); f piecewise(x>x^3 on (0, 1), x>x^2 on [1, 0]; x) sage: f(x=1/2)  TypeError Traceback (most recent call last) <ipythoninput26d710eda1bb0> in <module>() > 1 f(x=Integer(1)/Integer(2)) TypeError: new_f() got an unexpected keyword argument 'x'
I am not well versed in Python. I think there is maybe a problem in symbolic/function_factory.pyx:eval_on_operands()
(dynamic methods for symbolic expressions, added with #9556) which is uncovered with this ticket.
def eval_on_operands(f): @sage_wraps(f) def new_f(ex): return f(*ex.operands()) return new_f
Apparently f
gets some new_f
on creation and tries to call it with a named parameter but python barfs because the parameter wasn't expected in new_f
. Moreover, even without name f(1/2)
barfs because the number of arguments to new_f
doesn't fit anymore. So only f()
will call new_f
succesfully but that's useless.
comment:8 Changed 6 years ago by
There is new code for symbolic/function_factory.pyx:eval_on_operands()
in #14802, but if I use that I get TypeError: 'NoneType' object is not callable
for every call to f()
.
comment:9 followup: ↓ 11 Changed 6 years ago by
 Dependencies changed from #14800, #14780, #9556, #13125 to #14800, #14780, #9556, #13125, #14802
Ok, with the dependencies #13125 and #14802 there remain two failing doctests in piecewise.py
:
File "src/sage/functions/piecewise.py", line 493, in sage.functions.piecewise.PiecewiseFunction.EvaluationMethods.extension Failed example: g(3) ... File "<doctest sage.functions.piecewise.PiecewiseFunction.EvaluationMethods.extension[3]>", line 1, in <module> g(Integer(3)) File "/home/ralf/sage/local/lib/python2.7/sitepackages/sage/symbolic/function_factory.py", line 387, in new_f return f(ex, *new_args, **kwds) File "/home/ralf/sage/local/lib/python2.7/sitepackages/sage/functions/piecewise.py", line 398, in __call__ return self.subs(substitution) File "expression.pyx", line 4212, in sage.symbolic.expression.Expression.substitute (sage/symbolic/expression.cpp:20863) File "/home/ralf/sage/local/lib/python2.7/sitepackages/sage/functions/piecewise.py", line 209, in _subs_ raise ValueError('point is not in the domain') ValueError: point is not in the domain
and the same in line 498
, as well as more than 200 failing doctests in piecewiseold.py
because of the deprecation.
comment:10 Changed 6 years ago by
It appears that by checking RealSet.are_pairwise_disjoint(*domain_list)
in piecewise.py:145
, intervals with any boundaries from SR will not be accepted anymore.
age: piecewise([((1, sqrt(2)), x), ((sqrt(3), 2), x)], var=x)  ValueError Traceback (most recent call last) <ipythoninput27571b9720cf91> in <module>() > 1 piecewise([((Integer(1), sqrt(Integer(2))), x), ((sqrt(Integer(3)), Integer(2)), x)], var=x) /home/ralf/sage/local/lib/python2.7/sitepackages/sage/misc/lazy_import.so in sage.misc.lazy_import.LazyImport.__call__ (sage/misc/lazy_import.c:2696)() /home/ralf/sage/local/lib/python2.7/sitepackages/sage/functions/piecewise.pyc in __call__(self, function_pieces, **kwds) 143 domain_list.append(domain) 144 if not RealSet.are_pairwise_disjoint(*domain_list): > 145 raise ValueError('domains must be pairwise disjoint')
but, since the old class didn't bother, it accepted all input. So the doctests in the old class will fail because the old class is redirected to the new. Personally I would just remove all doctests from the old class instead of fiddling together 200 working ones for a deprecated class. Is this allowed?
In any case I consider the restriction to reals surprising. At least I would want to see a better error msg.
comment:11 in reply to: ↑ 9 Changed 6 years ago by
and the same in line
498
, as well as more than 200 failing doctests inpiecewiseold.py
because of the deprecation.
Yeah, I don't know how we are going to deal with this. I suspect we should have Piecewise
for the oldstyle one for a deprecation period and piecewise
for the new ones. The current changes here aren't really a deprecation at all, but rather a backwardsincompatible change.
comment:12 Changed 6 years ago by
 Branch changed from public/piecewise to public/piecewisealt
 Commit changed from 6d00b38418552984fa951bf9c4090da001a1ec9f to 81b6176243c4fc8dc5bf3dfd244dc34df2e5be6c
My solution would be to leave piecewise.py
as is and put the new code as piecewise_real = PiecewiseRealFunction()
into piecewise_real.py
. I have uploaded the alternative branch public/piecewisealt
to trac and set the Branch: field to it. Feel free to revert to the old one named public/piecewise
.
New commits:
32e801a  Import of trac_14802dynamic_attribute_args.patch from

b97156c  Implement RealSet  finite unions of open/closed/semiclosed subsets of the real line.

daf1c0c  Further improvements to RealSet

1b20c3b  Trac #13125: reviewer's patch: fix typos, add module to set refman

81b6176  fixes

comment:13 Changed 6 years ago by
 Commit changed from 81b6176243c4fc8dc5bf3dfd244dc34df2e5be6c to 852ba6f43b97dc143de2a3ec183d6770dfd885bc
comment:14 Changed 6 years ago by
 Status changed from new to needs_review
Now on to the remaining two errors. This code throws:
sage: f = piecewise_real([((1,1), x)]); f; g = f.extension(0); g; g(3)
It works however if I patch rings/real_lazy.pyx
:
diff git a/src/sage/sets/real_set.py b/src/sage/sets/real_set.py index adc41d3..1d43cc4 100644  a/src/sage/sets/real_set.py +++ b/src/sage/sets/real_set.py @@ 582,8 +582,8 @@ class RealInterval(UniqueRepresentation, Parent): sage: i.contains(2) True """  cmp_lower = cmp(self._lower, x)  cmp_upper = cmp(x, self._upper) + cmp_lower = cmp(self._lower._value, x) + cmp_upper = cmp(x, self._upper._value) if cmp_lower == cmp_upper == 1: return True if cmp_lower == 0:
So, as this fixes it and passes tests, I'm committing and proposing this branch for review.
New commits:
57e8254  fixes; remove crap

852ba6f  fix doctest; remove crap

comment:15 Changed 6 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:16 Changed 6 years ago by
 Commit changed from 852ba6f43b97dc143de2a3ec183d6770dfd885bc to e31ad6ba056270177f94d3a909fe6c527c992696
Branch pushed to git repo; I updated commit sha1. New commits:
e31ad6b  Merge branch 'develop' into t/14801/public/piecewisealt

comment:17 Changed 6 years ago by
Now that the comparisons with infinity are fixed we should be able to get this finished. I don't like the separate piecewise_real
thing. If there is some problem with symbolic expressions then that should be fixed. I'll have a look now.
comment:18 Changed 6 years ago by
 Branch changed from public/piecewisealt to public/piecewise
 Commit changed from e31ad6ba056270177f94d3a909fe6c527c992696 to 496128f8fad75a19945588139d887a5707a72214
 Dependencies changed from #14800, #14780, #9556, #13125, #14802 to #14800, #14780, #9556, #13125, #14802, #16397
comment:19 followup: ↓ 22 Changed 6 years ago by
It looks like there is still no proper deprecation for the "old" piecewise stuff. Given that there seems to be a significant amount of stuff that that has (even if in an antiquated form that should be redone), perhaps a potential solution would be to do a deprecation of Piecewise
that leads to the old version, but where piecewise
is the new one? (Wait, I already said this in comment:11.) I agree with Volker that piecewise_real
is probably too involved, but this is not so crucial a change that we shouldn't follow normal deprecation policy, especially since apparently people really do use the plotting and convolution/fourier stuff, buggy though it is.
comment:20 Changed 6 years ago by
Well the old branch should be still there, as I said in comment:12
comment:21 Changed 6 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:22 in reply to: ↑ 19 Changed 6 years ago by
Replying to kcrisman:
It looks like there is still no proper deprecation for the "old" piecewise stuff. Given that there seems to be a significant amount of stuff that that has (even if in an antiquated form that should be redone), perhaps a potential solution would be to do a deprecation of
Piecewise
that leads to the old version, but wherepiecewise
is the new one? (Wait, I already said this in comment:11.)
Seems that Volker already implemented it, just the doctests have to be adapted, and the cmp
stuff from #16397:
sage: R.<x> = PolynomialRing(QQ, 'x') sage: f = Piecewise([[(0,1),1*x^0]]) /home/ralf/sage/src/bin/sageipython:1: DeprecationWarning: use lowercase piecewise instead See http://trac.sagemath.org/14801 for details. #!/usr/bin/env python  sage t long src/sage/functions/piecewise.py # 3 doctests failed sage t long src/sage/functions/piecewiseold.py # 203 doctests failed
I think I'll put some work into it to reduce doctest failures a bit.
comment:23 Changed 6 years ago by
 Commit changed from 496128f8fad75a19945588139d887a5707a72214 to 262b01e0de35a35d46fedda5e99255a26b9b36fa
comment:24 Changed 6 years ago by
 Status changed from needs_review to needs_work
comment:25 followup: ↓ 26 Changed 6 years ago by
Could you please explain why we need to keep all the old stuff? What functionality/syntax worked with the old Piecewise
but not with the new piecewise
?
comment:26 in reply to: ↑ 25 Changed 6 years ago by
Replying to jdemeyer:
Could you please explain why we need to keep all the old stuff? What functionality/syntax worked with the old
Piecewise
but not with the newpiecewise
?
Not sure if we need these, they are demonstrated in the failing src/doc/en/constructions/calculus.rst
doctests:
convolution()
critical_points()
riemann_sum_integral_approximation()
trapezoid()
The rest of piecewiseold.py
appears not to be used elsewhere, or doesn't contribute to failing doctests.
comment:27 followups: ↓ 28 ↓ 34 Changed 6 years ago by
This is a bug:
sage: var('y') y sage: f(x) = piecewise([((0,2), y^3)]) sage: f(1) 1
The result should be y^3
as in:
sage: f(x) = y^3 sage: f(1) y^3
comment:28 in reply to: ↑ 27 ; followup: ↓ 29 Changed 6 years ago by
Replying to jdemeyer:
This is a bug:
sage: var('y') y sage: f(x) = piecewise([((0,2), y^3)]) sage: f(1) 1The result should be
y^3
as in:sage: f(x) = y^3 sage: f(1) y^3
Really?
sage: f(x) = piecewise([((0,2), y^3)],var=x) sage: f x > piecewise(x>y^3 on (0, 2); x) sage: f(1) y^3
comment:29 in reply to: ↑ 28 Changed 6 years ago by
Replying to rws:
sage: f(x) = piecewise([((0,2), y^3)],var=x)
I don't like that one need to write var=x
. I think the following should work somehow:
sage: f(x) = piecewise([((0,2), y^3)])
I understand the semantics are not easy, because you are using the variable twice: once for the condition on the domain (0 < x < 2
) and once to evaluate the function (y^3
). In a way, you want the result of piecewise(...)
to be like sin
but also like sin(x)
.
comment:30 followup: ↓ 31 Changed 6 years ago by
Given that
f(x) = foo
is preprocessor syntactic sugar for
f = foo.function(x)
the simplest solution might be to override the .function()
method to also change the variable name of the piecewise function.
comment:31 in reply to: ↑ 30 Changed 6 years ago by
Replying to jdemeyer:
Given that
f(x) = foois preprocessor syntactic sugar for
f = foo.function(x)the simplest solution might be to override the
.function()
method to also change the variable name of the piecewise function.
As far as I understand it, the only time symbolic_expr.function(..)
recurses into the expression is when converting into the CallableSymbolicExpressionRing
which calls SymbolicRing._element_constructor_()
. I don't think I can easily hand var
down to that machinery, recognize when a piecewise
get converted, and call a member func then.
IOW, the piecewise
on the r.h.s. of f(x) = piecewise([((0,2), y^3)])
may be buried within an expression, and I find it not the "simplest solution" to handle its var
via .function()
. But that's from a first look at parts of the code I have never worked with, so I would appreciate any hint.
comment:32 Changed 5 years ago by
Now that we have #17759 (for review) the tree can be walked conveniently and the variable setor so I thought, but the piecewise
object appears stateless: any call will reinitialize it...
comment:33 Changed 5 years ago by
 Commit changed from 262b01e0de35a35d46fedda5e99255a26b9b36fa to 80e8f85240fdb7904c1339d0438243abf1238f00
Branch pushed to git repo; I updated commit sha1. New commits:
b0ebe76  Merge branch 'develop' into t/14801/public/piecewise

3781eec  17759: convenience class symbolic ExpressionTreeWalker(Converter)

adb3798  Merge branch 'public/17759' of trac.sagemath.org:sage into t/14801/public/piecewise

80e8f85  14801: change piecewise var arg according to function var

comment:34 in reply to: ↑ 27 Changed 5 years ago by
 Dependencies changed from #14800, #14780, #9556, #13125, #14802, #16397 to #14800, #14780, #9556, #13125, #14802, #16397, #17759
Replying to jdemeyer:
The result should be
y^3
as in:sage: f(x) = y^3 sage: f(1) y^3
OK, this works now with one variable, dependent on #17759 (please review). I'm of the opinion this should work too:
sage: f(x,y) = piecewise([((0,2), y^3)]) sage: f(1,1) TypeError: __call__() takes at most 5 arguments (6 given)
but I have no idea atm what changes are needed for this. piecewise([((0,2), y^3)], var=(x,y))
is certainly not supported now.
comment:35 Changed 5 years ago by
Maybe the class needs redesign to support this format: piecewise([([(x,0,2),(y,0,2)], x*y^3)])
but I'd rather make this an enhancement.
comment:36 Changed 5 years ago by
Anyway, if no rectangular 2Dpieces are supported in this ticket then also no 2Dstrips are supported, i.e., this:
sage: f(x,y) = piecewise([((0,2), y^3)], var=x) sage: f(1,1) TypeError: __call__() takes at most 5 arguments (6 given)
maybe should raise an error already in the first command. Having specified what this ticket can do there remain these fails:
sage t src/sage/functions/piecewise.py # fast_callable, .extension() sage t src/doc/en/constructions/plotting.rst # TypeError sage t src/doc/en/constructions/calculus.rst # missing .critical_points(), .convolution(), trapezoid(), riemann_sum_integral_approximation(), .integral(), laplace(), fourier_series_cosine_coefficient(), TypeError sage t src/sage/calculus/wester.py # missing .integral()
EDIT: added problem issues
comment:37 Changed 5 years ago by
 Dependencies changed from #14800, #14780, #9556, #13125, #14802, #16397, #17759 to #14800, #14780, #9556, #13125, #14802, #16397, #17759, #17799
comment:38 Changed 5 years ago by
 Commit changed from 80e8f85240fdb7904c1339d0438243abf1238f00 to 3f5e8be61616c496aa9e9f41401a93a1cde0f9ff
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
5631c87  14801: critical points, convolution, end points

1e9eb43  14801: convolution

3c7876e  Merge branch 'develop' into t/14801/public/piecewise

64547f1  14801: old deprecation, doctests

ae65dc4  14801: piecewiseold, convolution

f146662  Merge branch 'develop' into t/14801/public/piecewise

566688b  17799: refactor real_set.py:RealInterval

0708a1a  Merge branch 't/17799/refactor_real_set_realinterval' into t/14801/public/piecewise

a6d48b7  14801: merge 17799

3f5e8be  14801: convolution

comment:39 Changed 5 years ago by
 Commit changed from 3f5e8be61616c496aa9e9f41401a93a1cde0f9ff to 254e047fc4056b06e2d2e4fe1296dad839549104
Branch pushed to git repo; I updated commit sha1. New commits:
254e047  14801: trapezoid; fixes

comment:40 Changed 5 years ago by
 Dependencies changed from #14800, #14780, #9556, #13125, #14802, #16397, #17759, #17799 to #14800, #14780, #9556, #13125, #14802, #16397, #17759, #17799, #17815
This leaves the ordering stuff, the plot warnings, and some Fourier code (in calculus.rst
).
comment:41 Changed 5 years ago by
 Milestone changed from sage6.4 to sage6.6
 Owner changed from burcin to rws
comment:42 Changed 5 years ago by
 Dependencies changed from #14800, #14780, #9556, #13125, #14802, #16397, #17759, #17799, #17815 to #14800, #14780, #9556, #13125, #14802, #16397, #17759, #17815, #12967
comment:43 Changed 5 years ago by
 Branch changed from public/piecewise to public/piecewise2
comment:44 Changed 5 years ago by
 Commit changed from 254e047fc4056b06e2d2e4fe1296dad839549104 to 1a0a27fc00100f78ee507ec2a73a6726bf063188
 Dependencies changed from #14800, #14780, #9556, #13125, #14802, #16397, #17759, #17815, #12967 to #14800, #14780, #9556, #13125, #14802, #16397, #17759, #17815
The remaining fails happen because:
 cannot use pi/2 as interval endpoint because of symbolic
cmp
bug (#16397)sage: cmp(pi/2,SR(0)) 1 sage: cmp(pi/2,0) 1
 unsuppressed doctest warnings (#17815)
 missing
piecewise.laplace
 missing
piecewise.fourier_series_sine_coefficient
 missing
piecewise.fourier_series_cosine_coefficient
 missing
piecewise.fourier_series_partial_sum
The last four are due to a single example in doc/en/constructions/calculus.rst
.
New commits:
4eb6c0f  14801: Piecewise functions done right  part 1

d733c7e  14801: Piecewise functions done right  part 2

9375510  Merge branch 'develop' into t/17759/public/17759

16aa81d  17759: handle hold=True and hypergeometric

cc19bd0  Merge branch 'public/17759' of trac.sagemath.org:sage into tmp5

1a0a27f  14801: fix doctest

comment:45 Changed 5 years ago by
 Commit changed from 1a0a27fc00100f78ee507ec2a73a6726bf063188 to c84b28a8f3f4776dd9e994e767cddf1fc4641541
Branch pushed to git repo; I updated commit sha1. New commits:
c84b28a  14801: lower verbosity in a doctest that became noisy

comment:46 Changed 5 years ago by
 Dependencies changed from #14800, #14780, #9556, #13125, #14802, #16397, #17759, #17815 to #14800, #14780, #9556, #13125, #14802, #16397, #17759
New commits:
c84b28a  14801: lower verbosity in a doctest that became noisy

comment:47 Changed 4 years ago by
 Cc slelievre added
comment:48 Changed 4 years ago by
 Description modified (diff)
Patch is just a first proof of concept now