Opened 3 years ago

Last modified 3 years ago

#25979 closed defect

py3: pynac bug with logb on int — at Version 10

Reported by: embray Owned by:
Priority: major Milestone: sage-8.7
Component: python3 Keywords:
Cc: rws Merged in:
Authors: Reviewers:
Report Upstream: Completely fixed; Fix reported upstream Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by embray)

On Python 2:

sage: from sage.functions.log import logb
sage: logb(7, 2)
sage: logb(int(7), 2)

On Python 3:

sage: from sage.functions.log import logb
sage: logb(7, 2)
sage: logb(int(7), 2)

This was causing quite a few confusing bugs before I realized the subtle difference of Integer vs int that was causing the bug. I'm not sure if it affect other functions too.

Upstream PR: (for pynac)

Though we should also still fix conversion of integer types to expressions on Python 3.

Change History (10)

comment:1 Changed 3 years ago by embray

It only seems to be if the first argument is an int; the type of the base doesn't seem to matter.

comment:2 Changed 3 years ago by jdemeyer

  • Cc rws added

comment:3 Changed 3 years ago by embray

Not 100% sure if related, but an interesting difference on Python 2 and 3:


sage: SR.coerce(Integer(7))._dbgprint()
7 (numeric) @0x603ced1f0, hash=0x7, flags=0x7, type MPZ
sage: SR.coerce(int(7))._dbgprint()
7 (numeric) @0x6016ab100, hash=0x7, flags=0x7, type LONG


sage: SR.coerce(Integer(7))._dbgprint()
7 (numeric) @0x4ce0820, hash=0x7, flags=0x7, type LONG
sage: SR.coerce(int(7))._dbgprint()
7 (numeric) @0x4ce0820, hash=0x7, flags=0x7, type MPZ

I don't think these are the exact same pynac versions either, so it's not clear to me yet where that difference is coming from.

comment:4 Changed 3 years ago by embray

Something like this seems to fix it, and makes more sense in terms of integer representation:

  • src/sage/symbolic/ring.pyx

    diff --git a/src/sage/symbolic/ring.pyx b/src/sage/symbolic/ring.pyx
    index 6ebb377..c9c44ee 100644
    a b The symbolic ring 
    1515from __future__ import absolute_import
     17from sage.arith.long cimport integer_check_long_py
    1718from sage.ext.cplusplus cimport ccrepr
    1920from sage.libs.pynac.pynac cimport *
    cdef class SymbolicRing(CommutativeRing): 
    331332            TypeError: positive characteristic not allowed in symbolic computations
    332333        """
    333334        cdef GEx exp
     335        cdef int err = 0
     336        cdef long x_long
    334338        if is_Expression(x):
    335339            if (<Expression>x)._parent is self:
    336340                return x
    cdef class SymbolicRing(CommutativeRing): 
    342346            try:
    343347                from sage.calculus.calculus import symbolic_expression_from_string
    344348                return self(symbolic_expression_from_string(x))
    345             except SyntaxError as err:
    346                 msg, s, pos = err.args
     349            except SyntaxError as exc:
     350                msg, s, pos = exc.args
    347351                raise TypeError("%s: %s !!! %s" %
    348352                        (msg, bytes_to_str(s[:pos]), bytes_to_str(s[pos:])))
    cdef class SymbolicRing(CommutativeRing): 
    361365                from sage.symbolic.constants import NaN
    362366                return NaN
    363367            exp = x
    364         elif isinstance(x, long):
    365             exp = x
    366         elif isinstance(x, int):
    367             exp = GEx(<long>x)
     368        elif integer_check_long_py(x, &x_long, &err):
     369            if not err:
     370                exp = GEx(x_long)
     371            else:
     372                exp = x
    368373        elif x is infinity:
    369374            return new_Expression_from_GEx(self, g_Infinity)
    370375        elif x is minus_infinity:

comment:5 Changed 3 years ago by embray

Despite the above fix, there is still an apparent problem:

sage: logb(2**30, 2)
sage: logb(2**31, 2)
sage: logb(2**32, 2)
sage: logb(2**33, 2)

...and so on.

comment:6 Changed 3 years ago by embray

I'm still getting:

sage: SR.coerce(Integer(2**30))._dbgprint()
1073741824 (numeric) @0x3b29c60, hash=0x40000000, flags=0x7, type LONG
sage: SR.coerce(Integer(2**31))._dbgprint()
2147483648 (numeric) @0x3b29c60, hash=0x80000000, flags=0x7, type MPZ

This is originating from ginac/numeric.cpp, specifically:

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

One, perhaps naïve, question I have is why mpz_fits_sint_p and not mpz_fits_slong_p?

comment:7 Changed 3 years ago by embray

So there's still a discrepancy between creating a pynac numeric from an Integer versus an int depending on what the value is. With my patch above, it will create a LONG type numeric up to int(2**63 - 1), or up to Integer(2**31 - 1).

But regardless, the logb function is broken for MPZ numerics.

comment:8 Changed 3 years ago by embray

I found it; small two character bug in pynac (and maybe upstream in ginac?):

  • ginac/numeric.cpp

    diff --git a/ginac/numeric.cpp b/ginac/numeric.cpp
    index 78643df..1b8a6cb 100644
    a b const numeric numeric::ratlog(const numeric &b, bool& israt) const { 
    37583758                return c;
    37593759        }
    37603760        if (b.t == LONG)
    3761                 return ratlog(to_bigint(), israt);
     3761                return ratlog(b.to_bigint(), israt);
    37623762        if (t == LONG)
    37633763                return to_bigint().ratlog(b, israt);
    37643764        if (t == MPZ and b.t == MPZ) {

comment:9 Changed 3 years ago by embray

  • Report Upstream changed from N/A to Not yet reported upstream; Will do shortly.

comment:10 Changed 3 years ago by embray

  • Description modified (diff)
Note: See TracTickets for help on using tickets.