Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug in convolution in old Piecewise #12123

Closed
kcrisman opened this issue Dec 6, 2011 · 13 comments
Closed

Bug in convolution in old Piecewise #12123

kcrisman opened this issue Dec 6, 2011 · 13 comments

Comments

@kcrisman
Copy link
Member

kcrisman commented Dec 6, 2011

The old Piecewise (pre #14801; now deprecated) has this bug:


sage: x = PolynomialRing(QQ,'x').gen()
sage: f = Piecewise([[(-2, 2), 2 * x^0]])
sage: g = Piecewise([[(0, 2), 3/4 * x^0]])
sage: n = f.convolution(g)

sage: n
Piecewise defined function with 3 parts, [[(-2, 0), 3/2*x + 3], [(0, 2), 6], [(2, 4), -3/2*x + 6]]

But the middle piece should be 3, not 6, apparently.

See the original report at this ask.sagemath.org question.

Note this is fixed in the new piecewise (lowercase p, from #14801).

sage: x = PolynomialRing(QQ,'x').gen()
sage: f = piecewise([[(-2, 2), 2 * x^0]])
sage: g = piecewise([[(0, 2), 3/4 * x^0]])
sage: n = f.convolution(g)
sage: n
piecewise(x|-->3/2*x + 3 on (-2, 0], x|-->3 on (0, 2], x|-->-3/2*x + 6 on (2, 4]; x)

Can close this bug when the old Piecewise is removed completely.

CC: @wdjoyner @kcrisman @jondo @vbraun @slel @mkoeppe @eviatarbach @rwst

Component: calculus

Keywords: piecewise

Stopgaps: todo

Reviewer: Travis Scrimshaw

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

@kcrisman kcrisman added this to the sage-5.11 milestone Dec 6, 2011
@kcrisman
Copy link
Member Author

kcrisman commented Dec 6, 2011

comment:1

The fix is to fix the following

    cmd1 = "integrate((%s)*(%s),%s,%s,%s)"%(i1,i2, uu, a1,    tt-b1)    ## if a1+b1 < tt < a2+b1
    cmd2 = "integrate((%s)*(%s),%s,%s,%s)"%(i1,i2, uu, tt-b2, tt-b1)    ## if a1+b2 < tt < a2+b1
    cmd3 = "integrate((%s)*(%s),%s,%s,%s)"%(i1,i2, uu, tt-b2, a2)       ## if a1+b2 < tt < a2+b2
    cmd4 = "integrate((%s)*(%s),%s,%s,%s)"%(i1,i2, uu, a1, a2)          ## if a2+b1 < tt < a1+b2
    <snip>
    if a1-b1<a2-b2:
        if a2+b1!=a1+b2:
           h = Piecewise([[(a1+b1,a1+b2),fg1],[(a1+b2,a2+b1),fg4],[(a2+b1,a2+b2),fg3]])

to have f2 instead of f4. There is a parallel part as well in the other branch of the if, where fg2 should be fg4, it appears.

This should be fixed quickly, but should also be checked to make sure it really does do the right thing! The code is not really commented enough to show what is going on with all these different mini-convolutions, one has to really think about it.

@jdemeyer jdemeyer modified the milestones: sage-5.11, sage-5.12 Aug 13, 2013
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.1, sage-6.2 Jan 30, 2014
@kcrisman
Copy link
Member Author

kcrisman commented Apr 3, 2014

comment:4

Here's another report that is almost certainly the same thing.

x = PolynomialRing(QQ, 'x').gen()
f1 = Piecewise([[(-1, 1), 1*x^0]])
f2 = Piecewise([[(0, 1), x], [(1, 2), -x + 2]])
g = f2.convolution(f1)
Piecewise defined function with 4 parts, [[(-1, 0), 1/2*x^2 + x +1/2], [(0, 1), -1/2*x^2 + 3*x], [(1, 2), -1/2*x^2 - x + 4], [(2, 3), 1/2*x^2 - 3*x + 9/2]].

but probably should be (according to the report)

Piecewise defined function with 3 parts, [[(-1, 0), 0.5*x^2 + x + 0.5], [(0, 2), -0.5*x^2 + x + 0.5], [(2, 3), 0.5*x^2 - 3*x + 4.5]

@kcrisman
Copy link
Member Author

kcrisman commented Apr 4, 2014

comment:5

In fact,

    if a1-b1<a2-b2:
        if a2+b1!=a1+b2:

doesn't even make sense; if the former is true, so is the latter? What on earth is going on here? I think an extra branch snuck in, in addition to the typo.

@kcrisman
Copy link
Member Author

kcrisman commented Apr 8, 2014

comment:6

See also #14801, though I don't know that will help with this.

@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.2, sage-6.3 May 6, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.3, sage-6.4 Aug 10, 2014
@kcrisman
Copy link
Member Author

comment:9

See #17793 (possible dup)

@rwst
Copy link

rwst commented Feb 19, 2015

comment:10

Replying to @kcrisman:

Here's another report that is almost certainly the same thing.

The two curve parts before addition look like this
<img src="http://s18.postimg.org/uptxg06pl/tmp_hd_MODX.png" 300px>
A working implementation should make sure that, when they are added, only three intervals remain.

sage: f = Piecewise([[(0, 1), -1/2*x^2 + x],[(1, 2), 1/2],[(2, 3), 1/2*x^2 - 3*x + 9/2]])
sage: g = Piecewise([[(-1, 0), 1/2*x^2 + x + 1/2],[(0, 1), 1/2],[(1, 2), -1/2*x^2 + x]])
sage: f+g
Piecewise defined function with 4 parts, [[(-1, 0), 1/2*x^2 + x + 1/2], [(0, 1), -1/2*x^2 + x + 1/2], [(1, 2), -1/2*x^2 + x + 1/2], [(2, 3), 1/2*x^2 - 3*x + 9/2]]

@sagetrac-jakobkroeker
Copy link
Mannequin

sagetrac-jakobkroeker mannequin commented Aug 25, 2015

Stopgaps: todo

@mkoeppe
Copy link
Member

mkoeppe commented Jun 25, 2016

Changed keywords from none to piecewise

@mkoeppe
Copy link
Member

mkoeppe commented Jun 25, 2016

comment:12

Description modified to comment that this seems fixed in the new piecewise (#14801).

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe modified the milestones: sage-6.4, sage-7.3 Jun 25, 2016
@mkoeppe mkoeppe changed the title Bug in convolution Bug in convolution in old Piecewise Jun 25, 2016
@fchapoton
Copy link
Contributor

comment:13

piecewise_old will be removed in #26865

@fchapoton fchapoton removed this from the sage-7.3 milestone Dec 10, 2018
@tscrim
Copy link
Collaborator

tscrim commented Dec 12, 2018

Reviewer: Travis Scrimshaw

@embray
Copy link
Contributor

embray commented Feb 26, 2019

comment:15

Presuming these are all correctly reviewed as either duplicate, invalid, or wontfix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants