a cython function that produces long given python input

[videlec]

I met twice the same problem which is the nedd for a standalone function that given a python objects return an associated long (if it makes sense).

The following is not safe

cdef long f(x):
    return x

since f(2/3) would return 0 (see e.g. #18278). Moreover, one could expect something that would be very fast for both Python int and Sage integers (see #18346).

CC: @nathanncohen

Component: coercion

Author: Vincent Delecroix

Branch/Commit: 6b46fee

Reviewer: Jeroen Demeyer

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

[jdemeyer]

comment:1

That's not a bug report, but a support request. If you post this to sage-support, I'll answer.

[videlec]

comment:3

Replying to @jdemeyer:

That's not a bug report, but a support request. If you post this to sage-support, I'll answer.

https://groups.google.com/forum/#!topic/sage-support/0ATjCgQlq4c

[videlec]

comment:4

In view of your answer on sage-support, it might still be worth to have something along

cdef inline long long_or_LONG_MIN(x):
    if PyInt_CheckExact(x):
        return PyInt_AS_LONG(x)
    elif type(x) is Integer:
        if mpz_fits_slong_p((<Integer>x).value):
            return mpz_get_si((<Integer>x).value)
        else:
            return LONG_MIN
    else:
        try:
            return PyNumber_Index(x)
        except Exception:
            return LONG_MIN 

It would be faster for Python ints and Sage integers.

Vincent

[jdemeyer]

comment:5

For Python ints, there will not be much difference (calling PyNumber_Index(x) on an int just returns x, so the only overhead is a function call and some reference counting).

For Integer, there is a larger gain. It's just annoying that Python doesn't allow defining a fast conversion ArbitraryType -> C long.

[nbruin]

comment:7

Replying to @jdemeyer:

For Integer, there is a larger gain. It's just annoying that Python doesn't allow defining a fast conversion ArbitraryType -> C long.

It does! It's called tp_hash :-). It's of course already taken for other purposes. We'd need an extra slot. Something along the lines of the conversion macro suggested in comment:4 would probably be the best we can do without it.

[jdemeyer]

comment:8

Replying to @nbruin:

We'd need an extra slot.

Sometimes I wish I could just patch Python...

Something along the lines of the conversion macro suggested in comment:4 would probably be the best we can do without it.

Indeed.

[videlec]

Branch: u/vdelecroix/18358

[videlec]

comment:9

Here is an experimental branch with a macro pyobject_to_long in sage.misc.long. I do not like the gestion of error but I do not know how to do better without affecting performance. Please tell me.

It is used in Integer.__pow__ and Rational.__pow__ as a proof of concept. And we win 20 precious nano seconds when the exponent is a small Sage Integer (~10%):

sage: timeit("2**5", number=400000, repeat=20)
400000 loops, best of 20: 248 ns per loop
sage: timeit("(2/3)**2", number=100000, repeat=20)
100000 loops, best of 20: 1.1 µs per loop

against

sage: timeit("2**5", number=400000, repeat=20)
400000 loops, best of 20: 228 ns per loop
sage: timeit("(2/3)**2", number=100000, repeat=20)
200000 loops, best of 30: 990 ns per loop

Vincent

---

New commits:

524393b	Trac 18290: upgrade sets and cartesian products
6e1e22d	Trac 18290: better doc + remove duplications
671e6ec	Trac 18290: fix doctest in combinat/tutorial
0378c4a	Trac 18290: fix doctest in categories/enumerated_sets.py
925b2e0	Trac 18290: fix doctests
8e91484	Trac 18358: macro to cast Python object to C long

[videlec]

Commit: 8e91484

[sagetrac-git]

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

b6df2ee	Trac 18358: macro pyobject -> long conversion
1e7db2e	Trac 18358: use macro in pow for Integer/Rational

[sagetrac-git]

Changed commit from 8e91484 to 1e7db2e

[jdemeyer]

comment:11

except LONG_MIN -> except? LONG_MIN and there is no need to document LONG_MIN in the docstring.

[jdemeyer]

comment:12

PyInt_CheckExact(...) -> isinstance(..., int)

[jdemeyer]

comment:13

OverflowError -> OverflowError("Integer too large to convert to C long") or something like that.

And obviously: doctests please.

[videlec]

comment:14

Replying to @jdemeyer:

OverflowError -> OverflowError("Integer too large to convert to C long") or something like that.

And obviously: doctests please.

How? Should I use Integer.__pow__ and Rational.__pow__ with indirect doctests?

[jdemeyer]

comment:16

It would make sense to merge 6.7.beta4 and replace all occurances of PyNumber_Index and PyNumber_AsSsize_t by this new function.

[videlec]

comment:17

Replying to @jdemeyer:

It would make sense to merge 6.7.beta4 and replace all occurances of PyNumber_Index and PyNumber_AsSsize_t by this new function.

It might not be good everywhere as the overflow might not be appropriate. I will have a look at occurrences.

[sagetrac-git]

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

f67ead5	Trac 18358: macro pyobject -> long conversion
98008da	Trac 18358: use macro in pow for Integer/Rational
c7b741f	Trac 18358: better pyobject_to_long
e7ad72f	Trac 18358: use pyobject_to_long where possible

[sagetrac-git]

Changed commit from 1e7db2e to e7ad72f

[videlec]

comment:19

Rebased on sage-6.7.beta4. I used pyobject_to_long where it seemed appropriate.

Vincent

[jdemeyer]

comment:20

In c_graph, if you call a variable u_long, actually make it a long (now it's still an int which means it can silently overflow).

[jdemeyer]

comment:21

You're not actually using this:

sage: tens = [10r, 10l, 10, 10/1]

[jdemeyer]

comment:22

I would prefer to replace

try:
    n = pyobject_to_long(exp)
except TypeError:
    raise TypeError("non-integral exponents not supported")

by

n = pyobject_to_long(exp)

[sagetrac-git]

Changed commit from e7ad72f to 90453ff

[sagetrac-git]

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

90453ff

Trac 18358: review commit

[videlec]

Reviewer: Jeroen Demeyer

[jdemeyer]

comment:25

Two more points:

1. raied -> raised

2. The doctests for Integer.__pow__ and Rational.__pow__ seem redundant. It's sufficient to keep just one.

[sagetrac-git]

Changed commit from 90453ff to 6b46fee

[sagetrac-git]

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

6b46fee

Trac #18358: typo + remove redundant doctest

[jdemeyer]

comment:29

I didn't run doctests again, but positive_review assuming that they pass.

[vbraun]

Changed branch from u/vdelecroix/18358 to 6b46fee