Opened 3 years ago
Closed 2 years 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, GitHub, GitLab) | Commit: | f96cd5318529b17cf85aef32252bc3614da7ac7e |
Dependencies: | Stopgaps: |
Description (last modified by )
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 3 years ago by
comment:2 Changed 3 years ago by
- Cc rws added
comment:3 Changed 3 years ago by
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 3 years ago by
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 14 14 #***************************************************************************** 15 15 from __future__ import absolute_import 16 16 17 from sage.arith.long cimport integer_check_long_py 17 18 from sage.ext.cplusplus cimport ccrepr 18 19 19 20 from sage.libs.pynac.pynac cimport * … … cdef class SymbolicRing(CommutativeRing): 331 332 TypeError: positive characteristic not allowed in symbolic computations 332 333 """ 333 334 cdef GEx exp 335 cdef int err = 0 336 cdef long x_long 337 334 338 if is_Expression(x): 335 339 if (<Expression>x)._parent is self: 336 340 return x … … cdef class SymbolicRing(CommutativeRing): 342 346 try: 343 347 from sage.calculus.calculus import symbolic_expression_from_string 344 348 return self(symbolic_expression_from_string(x)) 345 except SyntaxError as e rr:346 msg, s, pos = e rr.args349 except SyntaxError as exc: 350 msg, s, pos = exc.args 347 351 raise TypeError("%s: %s !!! %s" % 348 352 (msg, bytes_to_str(s[:pos]), bytes_to_str(s[pos:]))) 349 353 … … cdef class SymbolicRing(CommutativeRing): 361 365 from sage.symbolic.constants import NaN 362 366 return NaN 363 367 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 368 373 elif x is infinity: 369 374 return new_Expression_from_GEx(self, g_Infinity) 370 375 elif x is minus_infinity:
comment:5 Changed 3 years ago by
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 3 years ago by
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
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
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 { 3758 3758 return c; 3759 3759 } 3760 3760 if (b.t == LONG) 3761 return ratlog( to_bigint(), israt);3761 return ratlog(b.to_bigint(), israt); 3762 3762 if (t == LONG) 3763 3763 return to_bigint().ratlog(b, israt); 3764 3764 if (t == MPZ and b.t == MPZ) {
comment:9 Changed 3 years ago by
- Report Upstream changed from N/A to Not yet reported upstream; Will do shortly.
comment:10 Changed 3 years ago by
- Description modified (diff)
comment:11 Changed 3 years ago by
- Report Upstream changed from Not yet reported upstream; Will do shortly. to Reported upstream. No feedback yet.
comment:12 Changed 3 years ago by
- Report Upstream changed from Reported upstream. No feedback yet. to Fixed upstream, but not in a stable release.
comment:13 Changed 2 years ago by
- Milestone changed from sage-8.4 to sage-8.5
comment:14 Changed 2 years ago by
@rws, please make a pynac release
comment:15 Changed 2 years ago by
- Milestone changed from sage-8.5 to sage-8.7
Retargeting some of my tickets (somewhat optimistically for now).
comment:16 Changed 2 years ago by
see pynac update ticket in #26995
comment:17 Changed 2 years ago by
- 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 2 years ago by
- Branch set to u/chapoton/25979
- Commit set to f96cd5318529b17cf85aef32252bc3614da7ac7e
- Status changed from new to needs_review
comment:19 Changed 2 years ago by
- 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 2 years ago by
- Branch changed from u/chapoton/25979 to f96cd5318529b17cf85aef32252bc3614da7ac7e
- Resolution set to fixed
- Status changed from positive_review to closed
It only seems to be if the first argument is an
int
; the type of the base doesn't seem to matter.