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

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

Closed
DaveWitteMorris opened this issue Mar 10, 2021 · 33 comments
Closed

Comments

@DaveWitteMorris
Copy link
Member

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.

CC: @egourgoulhon @mjungmath

Component: symbolics

Keywords: pynac, expand

Author: Dave Morris

Branch/Commit: eeb6cc2

Reviewer: Volker Braun, Matthias Koeppe

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

@DaveWitteMorris
Copy link
Member Author

Branch: public/31479

@DaveWitteMorris
Copy link
Member Author

Commit: 3dfe9a6

@DaveWitteMorris
Copy link
Member Author

New commits:

371e10dtrac 31479: pynac's poly_mul_expand
3dfe9a6add doctests

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 10, 2021

comment:3

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
**********************************************************************

@DaveWitteMorris
Copy link
Member Author

comment:4

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?

@DaveWitteMorris
Copy link
Member Author

comment:5

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.

@vbraun
Copy link
Member

vbraun commented Mar 23, 2021

comment:8

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

@DaveWitteMorris
Copy link
Member Author

Changed branch from public/31479 to public/31479v2

@DaveWitteMorris
Copy link
Member Author

Changed commit from 3dfe9a6 to b6f9170

@DaveWitteMorris
Copy link
Member Author

comment:10

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

@vbraun
Copy link
Member

vbraun commented Mar 24, 2021

Reviewer: Volker Braun

@vbraun
Copy link
Member

vbraun commented Mar 28, 2021

comment:12

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
----------------------------------------------------------------------

@DaveWitteMorris
Copy link
Member Author

comment:13

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 5, 2021

Changed commit from 96b0f8b to 2364860

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 5, 2021

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

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 5, 2021

Dependencies: #31554

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 9, 2021

Changed commit from 2364860 to b0b074e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 9, 2021

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

@vbraun
Copy link
Member

vbraun commented Apr 10, 2021

comment:23

Circular dependency

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 10, 2021

Changed commit from b0b074e to 0f6dcc9

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 10, 2021

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

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 10, 2021

Changed dependencies from #31554 to none

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 10, 2021

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 10, 2021

Changed commit from 0f6dcc9 to eeb6cc2

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 10, 2021

comment:27

Sorry for messing this up.

@vbraun
Copy link
Member

vbraun commented Apr 12, 2021

Changed branch from public/31479v2 to eeb6cc2

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

3 participants