Opened 8 years ago
Closed 5 years ago
#14801 closed defect (fixed)
Piecewise functions done right
Reported by: | vbraun | Owned by: | rws |
---|---|---|---|
Priority: | major | Milestone: | sage-7.2 |
Component: | symbolics | Keywords: | |
Cc: | eviatarbach, kcrisman, slelievre, mkoeppe | Merged in: | |
Authors: | Volker Braun, Ralf Stephan | Reviewers: | Volker Braun, Ralf Stephan |
Report Upstream: | N/A | Work issues: | |
Branch: | 966859c (Commits, GitHub, GitLab) | Commit: | 966859c030256f78b656d9a7cb514364518fecc1 |
Dependencies: | #14800, #14780, #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/sage-devel/dgwUMsdiHfM
Attachments (1)
Change History (66)
comment:1 Changed 8 years ago by
comment:2 Changed 8 years ago by
- Dependencies set to #14800, #14780, #9556, #13125
comment:3 Changed 8 years ago by
- Cc eviatarbach added
comment:4 Changed 8 years ago by
- Cc kcrisman added
comment:5 Changed 7 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:6 Changed 7 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 7 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) <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.
comment:8 Changed 7 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 follow-up: ↓ 11 Changed 7 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/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.
comment:10 Changed 7 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) <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.
comment:11 in reply to: ↑ 9 Changed 7 years ago by
and the same in line
498
, as well as more than 200 failing doctests inpiecewise-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.
comment:12 Changed 7 years ago by
- Branch changed from public/piecewise to public/piecewise-alt
- 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/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
|
comment:13 Changed 7 years ago by
- Commit changed from 81b6176243c4fc8dc5bf3dfd244dc34df2e5be6c to 852ba6f43b97dc143de2a3ec183d6770dfd885bc
comment:14 Changed 7 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 7 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:16 Changed 7 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/piecewise-alt
|
comment:17 Changed 7 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 7 years ago by
- Branch changed from public/piecewise-alt 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 follow-up: ↓ 22 Changed 7 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 7 years ago by
Well the old branch should be still there, as I said in comment:12
comment:21 Changed 7 years ago by
- Milestone changed from sage-6.3 to sage-6.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/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.
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 follow-up: ↓ 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 piecewise-old.py
appears not to be used elsewhere, or doesn't contribute to failing doctests.
comment:27 follow-ups: ↓ 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 ; follow-up: ↓ 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 follow-up: ↓ 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 6 years ago by
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...
comment:33 Changed 6 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 6 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 6 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 6 years ago by
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
comment:37 Changed 6 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 6 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: 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
|
comment:39 Changed 6 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 6 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 6 years ago by
- Milestone changed from sage-6.4 to sage-6.6
- Owner changed from burcin to rws
comment:42 Changed 6 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 6 years ago by
- Branch changed from public/piecewise to public/piecewise-2
comment:44 Changed 6 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 6 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 6 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 5 years ago by
- Cc slelievre added
comment:48 Changed 5 years ago by
- Description modified (diff)
comment:49 Changed 5 years ago by
- Cc mkoeppe added
comment:50 Changed 5 years ago by
- Commit changed from c84b28a8f3f4776dd9e994e767cddf1fc4641541 to 64bbe5393ffc24ae3551644e5171692e7f7a2dd6
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
|
comment:51 Changed 5 years ago by
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.
comment:52 Changed 5 years ago by
Any ideas about how to resolve this?
comment:53 Changed 5 years ago by
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.
comment:54 Changed 5 years ago by
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.
comment:55 Changed 5 years ago by
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
.
comment:56 Changed 5 years ago by
- Commit changed from 64bbe5393ffc24ae3551644e5171692e7f7a2dd6 to b89ee3e56ee49d98a07a99d3b8741d6a60ba405c
comment:57 Changed 5 years ago by
- Milestone changed from sage-6.6 to sage-7.2
- Status changed from needs_work to needs_review
That should be it.
comment:58 Changed 5 years ago by
- Commit changed from b89ee3e56ee49d98a07a99d3b8741d6a60ba405c to 506248ad59a4e933f269c1f07865066a97e87a03
comment:59 Changed 5 years ago by
See also #8603 regarding the most recent additions - thanks for doing them.
comment:60 Changed 5 years ago by
- Commit changed from 506248ad59a4e933f269c1f07865066a97e87a03 to 966859c030256f78b656d9a7cb514364518fecc1
comment:61 Changed 5 years ago by
Thanks for working on it... I'm of course happy with your changes.
comment:62 Changed 5 years ago by
- Reviewers set to Volker Braun, Ralf Stephan
- Status changed from needs_review to positive_review
And I'm so with your part of this.
comment:63 Changed 5 years ago by
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.
comment:64 Changed 5 years ago by
- Dependencies changed from #14800, #14780, #9556, #13125, #14802, #16397, #17759 to #14800, #14780, #13125, #14802, #16397, #17759
comment:65 Changed 5 years ago by
- Branch changed from public/piecewise-2 to 966859c030256f78b656d9a7cb514364518fecc1
- Resolution set to fixed
- Status changed from positive_review to closed
Patch is just a first proof of concept now