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

Pynac should allow signed long variables to store large values #31986

Open
DaveWitteMorris opened this issue Jun 15, 2021 · 3 comments
Open

Pynac should allow signed long variables to store large values #31986

DaveWitteMorris opened this issue Jun 15, 2021 · 3 comments

Comments

@DaveWitteMorris
Copy link
Member

Pynac's numeric.cpp file includes the following function definition:

void set_from(Type& t, Value& v, long& hash, mpz_t bigint)
{
        if (mpz_fits_sint_p(bigint)) {
                t = LONG;
                v._long = mpz_get_si(bigint);
                hash = (v._long==-1) ? -2 : v._long;
        }
        else {
                t = MPZ;
                mpz_init_set(v._bigint, bigint);
                hash = _mpz_pythonhash(v._bigint);
        }
}

This means that if an integer value could fit into a C++ signed integer, then it will be stored as a signed long integer; otherwise, it will be stored as an mpz value. This is a waste of bits: a signed long integer can usually hold much larger values than a signed integer. Therefore, in order to use all of the bits of the signed long integer variable, this patch changes mpz_fits_sint_p to mpz_fits_slong_p.

This change may uncover bugs in the pynac code. For example, the bugs of #31585 were previously only seen on 32-bit, but they show up in 64-bit (after changing 2^31 to 2^63) if the patch here is applied without patches that solve the problems on that ticket:

sage: n = 2^63
sage: (2*x).subs(x=-n)
0
sage: abs(x).subs(x=-n)
-9223372036854775808
sage: (-x).subs(x=-n)
-9223372036854775808
sage: a,b,c,d = var("a b c d")
sage: ((a + b + c)^62 * (3*b + d - 5/d)^3).expand().subs(a=0,b=2,c=-1)
d^3 + 18*d^2 + 15439924789694894702685*d - 77199623948474473513425/d + 450/d^2 - 125/d^3 + 36
sage: (-n*x + 1)*x
------------------------------------------------------------------------
(no backtrace available)
------------------------------------------------------------------------
Unhandled SIGFPE: An unhandled floating point exception occurred.
This probably occurred because a *compiled* module has a bug
in it and is not properly wrapped with sig_on(), sig_off().
Python will now terminate.

It therefore seems that this ticket should be merged early in a release cycle, so I am suggesting 9.5 as the milestone.

Depends on #31585
Depends on #31984

Component: symbolics

Keywords: pynac, long

Branch/Commit: public/31986 @ 5e08324

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

@DaveWitteMorris
Copy link
Member Author

Branch: public/31986

@DaveWitteMorris
Copy link
Member Author

Commit: 5e08324

@DaveWitteMorris
Copy link
Member Author

comment:2

For now, if you want to try out the pynac patch on this ticket, you can merge the branch, and then ./sage -f pynac.


New commits:

5e08324trac 31986 allow long values in pynac

@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Dec 18, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.6, sage-9.7 Apr 1, 2022
@mkoeppe mkoeppe modified the milestones: sage-9.7, sage-9.8 Aug 31, 2022
@mkoeppe mkoeppe removed this from the sage-9.8 milestone Jan 29, 2023
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

2 participants