Opened 10 months ago

Closed 3 months ago

#25979 closed defect (fixed)

py3: pynac bug with logb on int

Reported by: embray Owned by:
Priority: major Milestone: sage-8.7
Component: python3 Keywords:
Cc: rws Merged in:
Authors: Frédéric Chapoton Reviewers: Travis Scrimshaw
Report Upstream: Completely fixed; Fix reported upstream Work issues:
Branch: f96cd53 (Commits) Commit: f96cd5318529b17cf85aef32252bc3614da7ac7e
Dependencies: Stopgaps:

Description (last modified by embray)

On Python 2:

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

On Python 3:

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

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: https://github.com/pynac/pynac/pull/336 (for pynac)

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

Change History (20)

comment:1 Changed 10 months 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 10 months ago by jdemeyer

  • Cc rws added

comment:3 Changed 10 months ago by embray

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

py2:

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

py3:

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 10 months 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 
    1414#*****************************************************************************
    1515from __future__ import absolute_import
    1616
     17from sage.arith.long cimport integer_check_long_py
    1718from sage.ext.cplusplus cimport ccrepr
    1819
    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
     337
    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:])))
    349353
    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 10 months ago by embray

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

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

...and so on.

comment:6 Changed 10 months 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 10 months 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 10 months 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 10 months ago by embray

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

comment:10 Changed 10 months ago by embray

  • Description modified (diff)

comment:11 Changed 10 months ago by embray

  • Report Upstream changed from Not yet reported upstream; Will do shortly. to Reported upstream. No feedback yet.

comment:12 Changed 10 months ago by embray

  • Report Upstream changed from Reported upstream. No feedback yet. to Fixed upstream, but not in a stable release.

comment:13 Changed 7 months ago by embray

  • Milestone changed from sage-8.4 to sage-8.5

comment:14 Changed 5 months ago by chapoton

@rws, please make a pynac release

comment:15 Changed 5 months ago by embray

  • Milestone changed from sage-8.5 to sage-8.7

Retargeting some of my tickets (somewhat optimistically for now).

comment:16 Changed 5 months ago by chapoton

see pynac update ticket in #26995

comment:17 Changed 4 months ago by chapoton

  • Report Upstream changed from Fixed upstream, but not in a stable release. to Fixed upstream, in a later stable release.

one still needs to add a doctest ?

comment:18 Changed 3 months ago by chapoton

  • Branch set to u/chapoton/25979
  • Commit set to f96cd5318529b17cf85aef32252bc3614da7ac7e
  • Status changed from new to needs_review

doctest added


New commits:

f96cd53add one doctest for logb

comment:19 Changed 3 months ago by tscrim

  • Authors set to Frédéric Chapoton
  • Report Upstream changed from Fixed upstream, in a later stable release. to Completely fixed; Fix reported upstream
  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM.

comment:20 Changed 3 months ago by vbraun

  • Branch changed from u/chapoton/25979 to f96cd5318529b17cf85aef32252bc3614da7ac7e
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.