Piecewise functions done right

[vbraun]

Rewrite piecewise functions as symbolic functions.

For a (late) discussion about interface issues see ​https://groups.google.com/forum/?hl=en#!topic/sage-devel/dgwUMsdiHfM

[vbraun]

Patch is just a first proof of concept now

[vbraun]

Updated patch

[kcrisman]

Presumably this would (help) solve #11225, #1773, #8994, and #8603. And #12123. Or maybe this won't help if it removes support for those things...

See also ​Maxima's pw.mac.

[rws]

New commits:

​6d00b38

Trac 14801: Rewrite piecewise functions as symbolic functions

[rws]

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)
<ipython-input-2-6d710eda1bb0> 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.

[rws]

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

[rws]

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/site-packages/sage/symbolic/function_factory.py", line 387, in new_f
        return f(ex, *new_args, **kwds)
      File "/home/ralf/sage/local/lib/python2.7/site-packages/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/site-packages/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 piecewise-old.py because of the deprecation.

[rws]

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)
<ipython-input-27-571b9720cf91> 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/site-packages/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/site-packages/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.

[kcrisman]

and the same in line 498, as well as more than 200 failing doctests in piecewise-old.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 old-style 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 backwards-incompatible change.

[rws]

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/piecewise-alt 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_14802-dynamic_attribute_args.patch from
​b97156c	Implement RealSet - finite unions of open/closed/semi-closed 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

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​57e8254	fixes; remove crap
​852ba6f	fix doctest; remove crap

[rws]

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

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​e31ad6b

Merge branch 'develop' into t/14801/public/piecewise-alt

[vbraun]

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.

[vbraun]

New commits:

​6d00b38	Trac 14801: Rewrite piecewise functions as symbolic functions
​496128f	Merge branch 'develop' into t/14801/public/piecewise

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

[rws]

Well the old branch should be still there, as I said in comment:12

[rws]

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 where piecewise 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/sage-ipython:1: DeprecationWarning: use lower-case 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/piecewise-old.py  # 203 doctests failed

I think I'll put some work into it to reduce doctest failures a bit.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​068c9e0	Merge branch 'develop' into t/14801/public/piecewise
​262b01e	14801: fix 51 doctests with deprecation warnings

[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 new piecewise?

[rws]

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 new piecewise?

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 piecewise-old.py appears not to be used elsewhere, or doesn't contribute to failing doctests.

[jdemeyer]

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

[rws]

Replying to jdemeyer:

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

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

[jdemeyer]

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

[jdemeyer]

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.

[rws]

Replying to jdemeyer:

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.

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.

[rws]

Now that we have #17759 (for review) the tree can be walked conveniently and the variable set---or so I thought, but the piecewise object appears stateless: any call will reinitialize it...

[git]

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

[rws]

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.

[rws]

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.

[rws]

Anyway, if no rectangular 2D-pieces are supported in this ticket then also no 2D-strips 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

[git]

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: piecewise-old, 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

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​254e047

14801: trapezoid; fixes

[rws]

This leaves the ordering stuff, the plot warnings, and some Fourier code (in calculus.rst).

[rws]

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

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​c84b28a

14801: lower verbosity in a doctest that became noisy

[rws]

New commits:

​c84b28a

14801: lower verbosity in a doctest that became noisy

[git]

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

​06ea5a9	fix doctest
​c78d151	add Constant.__lt__
​1381a89	16397: add tests
​585ceb8	Merge branch 'develop' into symbcmp
​60370a9	16397: fix typo
​f78854c	fix depecrated import of python.pxi
​eab9ec2	16397: fix doctest
​24450f8	Merge branch 'public/16397-1' of git://trac.sagemath.org/sage into tmp04
​07f12cd	16397: restore transitivity
​64bbe53	Merge branch 'public/piecewise-2' of git://trac.sagemath.org/sage into piecewise

[rws]

Indeed, as the discussion in ​https://groups.google.com/forum/?hl=en#!topic/sage-devel/dgwUMsdiHfM foresaw, lambdas will be a problem with this, as they cannot be coerced to SR.

sage: f1 = lambda x: -1; f2 = lambda x: 2                   
sage: piecewise([((0,pi/2),f1), ((pi/2,pi),f2)])
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

There is one doctest in src/doc/en/constructions/calculus.rst that requires it.

But it doesn't even work with NewSymbolicFunction:

sage: function('newf2')(x)                      
newf2(x)
sage: piecewise([((0,pi/2),x), ((pi/2,pi),f2)])

for which there seems a solution in #17701.

[rws]

Any ideas about how to resolve this?

[rws]

As #16397 is accepted let's summarize the current situation. There are two doctests failing with just a different number of graphics objects, easily fixed, which leaves five fails in doc/en/constructions/calculus.rst due to:

• missing piecewise.laplace
• missing piecewise.fourier_series_sine_coefficient
• missing piecewise.fourier_series_cosine_coefficient
• missing piecewise.fourier_series_partial_sum
• lambdas not coercible into SR (line 381)

As to the last item I think the doctest could be changed to no longer use a lambda. Users depending on it could use piecewise_old. We could make this ticket dependent on #17701 to at least have anonymous symbolic functions working as in function('newf2')(x).

If people insist on lambdas and def'ed Python functions as arguments to piecewise it might be an idea to create a temporary anonymous function (check for types.FunctionType) but this depends on #17701 if the idea can be made to work.

[rws]

The Laplace transform code as well depends on function arguments being coercible/convertible to SR: result = sum([(SR(f)*exp(-s*x)).integral(x,a,b) for (a,b),f in self.list()]). I don't think there is an easy solution and I'm wondering why the new piecewise should come up with something that wasn't resolved with other parts of Sage, like diff, desolve, integral etc.

[rws]

The solution is to change the doctest to:

    sage: f1 = lambda x: -1
    sage: f2 = lambda x: 2
    sage: f = piecewise([((0,pi/2),f1(x)), ((pi/2,pi),f2(x))])

Or, if function objects are used to apply the call inside piecewise.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​e0b44e4	Merge branch 'develop' into t/14801/public/piecewise-2
​b89ee3e	14801: last part of additions and fixes

[rws]

That should be it.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​fab81a4	Merge branch 'develop' into t/14801/public/piecewise-2
​506248a	doctest fixes

[kcrisman]

See also #8603 regarding the most recent additions - thanks for doing them.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​a7c8bb9	Use items instead of iteritems (py3)
​67ccb8a	Complete piecewise.py coverage
​966859c	Fix raise statement (py3)

[vbraun]

Thanks for working on it... I'm of course happy with your changes.

[rws]

And I'm so with your part of this.

[wdj]

Ralf and Volker have done such great work on this and I really am very much looking forward to this being put into Sage. Also, I don't see any examples which used to work but don't work with the new version you've made.

A comment on how I tested this (since I don't know git), I downloaded your piecewise.py file from git. Added it to a new version of SageMath (since "sage -clone" no longer works) and ran "sage -b". Then I started sage and just ran then examples in the docstring. I tweeked several just to see how minor changes affected things. They all went perfectly.