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

Depends on #14800
Depends on #14780
Depends on #13125
Depends on #14802
Depends on #16397
Depends on #17759

CC: @eviatarbach @kcrisman @slel @mkoeppe

Component: symbolics

Author: Volker Braun, Ralf Stephan

Branch/Commit: 966859c

Reviewer: Volker Braun, Ralf Stephan

Issue created by migration from https://trac.sagemath.org/ticket/14801

[vbraun]

comment:1

Patch is just a first proof of concept now

[vbraun]

Dependencies: #14800, #14780, #9556, #13125

[vbraun]

Updated patch

[eviatarbach]

comment:3

Attachment: trac_14801_piecewiese.patch.gz

[kcrisman]

comment:4

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.

[rwst]

New commits:

6d00b38

Trac 14801: Rewrite piecewise functions as symbolic functions

[rwst]

Commit: 6d00b38

[rwst]

Branch: public/piecewise

[rwst]

comment:7

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.

[rwst]

comment:8

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

[rwst]

comment:9

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.

[rwst]

Changed dependencies from #14800, #14780, #9556, #13125 to #14800, #14780, #9556, #13125, #14802

[rwst]

comment:10

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]

comment:11

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.

[rwst]

Changed author from Volker Braun to Volker Braun, Ralf Stephan

[rwst]

comment:12

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

[rwst]

Changed commit from 6d00b38 to 81b6176

[rwst]

Changed branch from public/piecewise to public/piecewise-alt

[sagetrac-git]

Changed commit from 81b6176 to 852ba6f

[sagetrac-git]

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

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

[rwst]

comment:14

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

[rwst]

comment:55

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.

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

[sagetrac-git]

Changed commit from 64bbe53 to b89ee3e

[rwst]

comment:57

That should be it.

[sagetrac-git]

Changed commit from b89ee3e to 506248a

[sagetrac-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]

comment:59

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

[sagetrac-git]

Changed commit from 506248a to 966859c

[sagetrac-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]

comment:61

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

[rwst]

Reviewer: Volker Braun, Ralf Stephan

[rwst]

comment:62

And I'm so with your part of this.

[wdjoyner]

comment:63

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.

[vbraun]

Changed dependencies from #14800, #14780, #9556, #13125, #14802, #16397, #17759 to #14800, #14780, #13125, #14802, #16397, #17759

[vbraun]

Changed branch from public/piecewise-2 to 966859c