Opened 7 years ago

Closed 3 weeks ago

#12123 closed defect (invalid)

Bug in convolution in old Piecewise

Reported by: kcrisman Owned by: burcin
Priority: minor Milestone: sage-duplicate/invalid/wontfix
Component: calculus Keywords: piecewise
Cc: wdj, kcrisman, jondo, vbraun, slelievre, mkoeppe, eviatarbach, rws Merged in:
Authors: Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps: todo

Description (last modified by mkoeppe)

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.

Change History (15)

comment:1 Changed 7 years ago by kcrisman

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.

comment:2 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:3 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:4 follow-up: Changed 5 years ago by kcrisman

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]

comment:5 Changed 5 years ago by kcrisman

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.

comment:6 Changed 5 years ago by kcrisman

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

comment:7 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:8 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:9 Changed 4 years ago by kcrisman

See #17793 (possible dup)

comment:10 in reply to: ↑ 4 Changed 4 years ago by rws

Replying to kcrisman:

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

The two curve parts before addition look like this http://s18.postimg.org/uptxg06pl/tmp_hd_MODX.png 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]]
Version 0, edited 4 years ago by rws (next)

comment:11 Changed 4 years ago by jakobkroeker

  • Stopgaps set to todo

comment:12 Changed 3 years ago by mkoeppe

  • Cc kcrisman jondo vbraun slelievre mkoeppe eviatarbach rws added
  • Description modified (diff)
  • Keywords piecewise added
  • Milestone changed from sage-6.4 to sage-7.3
  • Priority changed from major to minor
  • Summary changed from Bug in convolution to Bug in convolution in old Piecewise

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

comment:13 Changed 3 months ago by chapoton

  • Milestone changed from sage-7.3 to sage-duplicate/invalid/wontfix
  • Status changed from new to needs_review

piecewise_old will be removed in #26865

Last edited 3 months ago by chapoton (previous) (diff)

comment:14 Changed 3 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

comment:15 Changed 3 weeks ago by embray

  • Resolution set to invalid
  • Status changed from positive_review to closed

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

Note: See TracTickets for help on using tickets.