Opened 19 months ago
Closed 18 months ago
#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:  sage9.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: 
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 reenabled after the bugs have been fixed.
Change History (28)
comment:1 Changed 19 months ago by
Branch:  → public/31479 

comment:2 Changed 19 months ago by
Commit:  → 3dfe9a6785dfc664317031f03ddfc9a778787de3 

Status:  new → needs_review 
comment:3 Changed 19 months ago by
Status:  needs_review → needs_work 

Patchbot indicates doctest failures:
sage t long warnlong 68.0 randomseed=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
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
Status:  needs_work → needs_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
Cc:  Eric Gourgoulhon added 

comment:7 Changed 18 months ago by
Cc:  Michael Jung added 

comment:8 Changed 18 months ago by
Status:  needs_review → needs_work 

The patch level in build/pkgs/pynac/packageversion.txt needs to be incremented so the build system knows that pynac must be rebuilt...
comment:9 Changed 18 months ago by
Branch:  public/31479 → public/31479v2 

comment:10 Changed 18 months ago by
Commit:  3dfe9a6785dfc664317031f03ddfc9a778787de3 → b6f91708881e26205e362c3461614d2e5b88fc78 

Status:  needs_work → needs_review 
comment:11 Changed 18 months ago by
Reviewers:  → Volker Braun 

Status:  needs_review → positive_review 
comment:12 Changed 18 months ago by
Status:  positive_review → needs_work 

On 32bit
********************************************************************** 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 randomseed=0 src/sage/symbolic/expression.pyx # 1 doctest failed 
comment:13 Changed 18 months ago by
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 32bit version of this doctest # known bug
and open a new (critical) ticket to fix the overflow error.
comment:14 Changed 18 months ago by
Volker, given that you seem to be the only one who has access to a 32bit machine, perhaps you can help locating this bug?
comment:15 Changed 18 months ago by
Commit:  b6f91708881e26205e362c3461614d2e5b88fc78 → 96b0f8bd3c791f533989d92d026e9558fa880a5c 

Branch pushed to git repo; I updated commit sha1. New commits:
96b0f8b  add 32bit known bug tags

comment:16 Changed 18 months ago by
Status:  needs_work → needs_review 

I opened #31585 for the integer overflow error.
comment:17 Changed 18 months ago by
Reviewers:  Volker Braun → Volker Braun, Matthias Koeppe 

Status:  needs_review → positive_review 
Short of new information regarding the 32bit 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:19 Changed 18 months ago by
Commit:  96b0f8bd3c791f533989d92d026e9558fa880a5c → 23648607ab7d56187c8da7baa1ca16ac782f5244 

Branch pushed to git repo; I updated commit sha1. New commits:
4bacd76  trac 31554 do not use handle_factor

00b44eb  add doctest

707560a  increment pynac patch number

82843fe  remove debugging code

ed6cac2  Merge branch 'public/31554' of git://trac.sagemath.org/sage into t/31479/public/31479v2

2364860  build/pkgs/pynac/packageversion.txt: Bump patch level

comment:20 Changed 18 months ago by
Dependencies:  → #31554 

Status:  needs_work → positive_review 
comment:21 Changed 18 months ago by
Commit:  23648607ab7d56187c8da7baa1ca16ac782f5244 → b0b074e5bbd1ac31c29af7b08a274a7c313a28af 

Status:  positive_review → needs_review 
comment:22 Changed 18 months ago by
Status:  needs_review → positive_review 

comment:24 Changed 18 months ago by
Commit:  b0b074e5bbd1ac31c29af7b08a274a7c313a28af → 0f6dcc93996d4f6bb23256b7c7811a1c0e81ed70 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
comment:25 Changed 18 months ago by
Dependencies:  #31554 

Status:  needs_work → needs_review 
comment:26 Changed 18 months ago by
Commit:  0f6dcc93996d4f6bb23256b7c7811a1c0e81ed70 → eeb6cc28b30923aeaaf26a4375683e20ee5c27df 

Branch pushed to git repo; I updated commit sha1. New commits:
eeb6cc2  build/pkgs/pynac/packageversion.txt: Bump patch level

comment:27 Changed 18 months ago by
Status:  needs_review → positive_review 

Sorry for messing this up.
comment:28 Changed 18 months ago by
Branch:  public/31479v2 → eeb6cc28b30923aeaaf26a4375683e20ee5c27df 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
trac 31479: pynac's poly_mul_expand
add doctests