#31479 closed defect (fixed)

Do not use pynac's poly_mul_expand function until it has been debugged

Reported by: Dave Morris Owned by:
Priority: blocker Milestone: sage-9.3
Component: symbolics Keywords: pynac, expand
Cc: Eric Gourgoulhon, Michael Jung Merged in:
Authors: Dave Morris Reviewers: Volker Braun, Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: eeb6cc2 (Commits, GitHub, GitLab) Commit: eeb6cc28b30923aeaaf26a4375683e20ee5c27df
Dependencies: Stopgaps:

Status badges

Description

Pynac's poly_mul_expand function silently produces wrong results when expanding certain sums. This ticket eliminates its use by pynac's ex::expand method, and therefore provides a (temporary) fix for the problems reported in #31077 and #31411.

This is part of metaticket #31478.

Use of the poly_mul_expand function can be re-enabled after the bugs have been fixed.

Change History (28)

comment:1 Changed 19 months ago by Dave Morris

Branch: public/31479

comment:2 Changed 19 months ago by Dave Morris

Commit: 3dfe9a6785dfc664317031f03ddfc9a778787de3
Status: newneeds_review

New commits:

371e10dtrac 31479: pynac's poly_mul_expand
3dfe9a6add doctests

comment:3 Changed 19 months ago by Matthias Köppe

Status: needs_reviewneeds_work

Patchbot indicates doctest failures:

sage -t --long --warn-long 68.0 --random-seed=0 src/sage/symbolic/expression.pyx
**********************************************************************
File "src/sage/symbolic/expression.pyx", line 4846, in sage.symbolic.expression.Expression.expand
Failed example:
    ((a + b + c)^30 * (3*b + d - 5/d)^3).expand().subs(a=0,b=2,c=-1)
Expected:
    d^3 + 18*d^2 + 93*d - 465/d + 450/d^2 - 125/d^3 + 36
Got:
    -124*d^3 + 468*d^2 - 372*d + 36
**********************************************************************
File "src/sage/symbolic/expression.pyx", line 4854, in sage.symbolic.expression.Expression.expand
Failed example:
    bool((A * B).expand() == (A * B.expand()).expand())
Expected:
    True
Got:
    False
**********************************************************************

comment:4 Changed 19 months ago by Dave Morris

The doctests that failed are the new ones that are added by this ticket. They will fail without the pynac patch that is on this ticket, but will pass if the patch is applied.

I am pretty sure that the patchbot did not apply the patch to pynac. Is there something I should do to make the patchbot apply the patch?

comment:5 Changed 19 months ago by Dave Morris

Status: needs_workneeds_review

Doctesting this ticket on MacOS 10.15.7 with

sage -f pynac
make
sage -t -p 4 --all --long

resulted in All tests passed!. I also tested on Ubuntu 20.04 (CoCalc) and got no failures that I would attribute to this ticket. So I think we can consider the badge to be green, even though the patchbots aren't testing the pynac patch.

comment:6 Changed 18 months ago by Matthias Köppe

Cc: Eric Gourgoulhon added

comment:7 Changed 18 months ago by Matthias Köppe

Cc: Michael Jung added

comment:8 Changed 18 months ago by Volker Braun

Status: needs_reviewneeds_work

The patch level in build/pkgs/pynac/package-version.txt needs to be incremented so the build system knows that pynac must be rebuilt...

comment:9 Changed 18 months ago by Dave Morris

Branch: public/31479public/31479v2

comment:10 Changed 18 months ago by Dave Morris

Commit: 3dfe9a6785dfc664317031f03ddfc9a778787de3b6f91708881e26205e362c3461614d2e5b88fc78
Status: needs_workneeds_review

Thanks! I incremented the patch number (and rebased on beta9), so I hope the patchbots will start chiming in.


New commits:

270a2f9trac 31479: pynac's poly_mul_expand
a2ebbebadd doctests
b6f9170increment package version

comment:11 Changed 18 months ago by Volker Braun

Reviewers: Volker Braun
Status: needs_reviewpositive_review

comment:12 Changed 18 months ago by Volker Braun

Status: positive_reviewneeds_work

On 32-bit

**********************************************************************
File "src/sage/symbolic/expression.pyx", line 4882, in sage.symbolic.expression.Expression.expand
Failed example:
    ((a + b + c)^30 * (3*b + d - 5/d)^3).expand().subs(a=0,b=2,c=-1)
Expected:
    d^3 + 18*d^2 + 93*d - 465/d + 450/d^2 - 125/d^3 + 36
Got:
    d^3 + 18*d^2 + 1739461754973*d - 8697308774865/d + 450/d^2 - 125/d^3 + 36
**********************************************************************
1 item had failures:
   1 of  43 in sage.symbolic.expression.Expression.expand
    [2937 tests, 1 failure, 38.78 s]
----------------------------------------------------------------------
sage -t --long --random-seed=0 src/sage/symbolic/expression.pyx  # 1 doctest failed
----------------------------------------------------------------------

comment:13 Changed 18 months ago by Dave Morris

The answer is correct modulo 2^32, so it seems clear that this is an integer overflow error. But pynac is not supposed to make that type of error, so I don't know what is going on. It may not be difficult to locate the bug on a machine that reproduces the error, but I don't have one right now.

I propose to mark the 32-bit version of this doctest # known bug and open a new (critical) ticket to fix the overflow error.

comment:14 Changed 18 months ago by Matthias Köppe

Volker, given that you seem to be the only one who has access to a 32-bit machine, perhaps you can help locating this bug?

comment:15 Changed 18 months ago by git

Commit: b6f91708881e26205e362c3461614d2e5b88fc7896b0f8bd3c791f533989d92d026e9558fa880a5c

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

96b0f8badd 32-bit known bug tags

comment:16 Changed 18 months ago by Dave Morris

Status: needs_workneeds_review

I opened #31585 for the integer overflow error.

comment:17 Changed 18 months ago by Matthias Köppe

Reviewers: Volker BraunVolker Braun, Matthias Koeppe
Status: needs_reviewpositive_review

Short of new information regarding the 32-bit status (is the failure introduced by this ticket? or just not fixed by this ticket?), I'm marking this as positive review, as it is certainly an improvement. (Volker, note, if you consider 32 bit support important, let's please merge #31541 too in this release too.)

comment:18 Changed 18 months ago by Volker Braun

Status: positive_reviewneeds_work

Merge conflict

comment:19 Changed 18 months ago by git

Commit: 96b0f8bd3c791f533989d92d026e9558fa880a5c23648607ab7d56187c8da7baa1ca16ac782f5244

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

4bacd76trac 31554 do not use handle_factor
00b44ebadd doctest
707560aincrement pynac patch number
82843feremove debugging code
ed6cac2Merge branch 'public/31554' of git://trac.sagemath.org/sage into t/31479/public/31479v2
2364860build/pkgs/pynac/package-version.txt: Bump patch level

comment:20 Changed 18 months ago by Matthias Köppe

Dependencies: #31554
Status: needs_workpositive_review

comment:21 Changed 18 months ago by git

Commit: 23648607ab7d56187c8da7baa1ca16ac782f5244b0b074e5bbd1ac31c29af7b08a274a7c313a28af
Status: positive_reviewneeds_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

0f6dcc9Merge tag '9.3.rc2' into t/31554/public/31554
b0b074eMerge #31479

comment:22 Changed 18 months ago by Matthias Köppe

Status: needs_reviewpositive_review

comment:23 Changed 18 months ago by Volker Braun

Status: positive_reviewneeds_work

Circular dependency

comment:24 Changed 18 months ago by git

Commit: b0b074e5bbd1ac31c29af7b08a274a7c313a28af0f6dcc93996d4f6bb23256b7c7811a1c0e81ed70

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

comment:25 Changed 18 months ago by Matthias Köppe

Dependencies: #31554
Status: needs_workneeds_review

comment:26 Changed 18 months ago by git

Commit: 0f6dcc93996d4f6bb23256b7c7811a1c0e81ed70eeb6cc28b30923aeaaf26a4375683e20ee5c27df

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

eeb6cc2build/pkgs/pynac/package-version.txt: Bump patch level

comment:27 Changed 18 months ago by Matthias Köppe

Status: needs_reviewpositive_review

Sorry for messing this up.

comment:28 Changed 18 months ago by Volker Braun

Branch: public/31479v2eeb6cc28b30923aeaaf26a4375683e20ee5c27df
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.