rewrite Matrix_modn_sparse and vector_modn_sparse code so that the modulus is 64-bit on 64-bit platforms

[williamstein]

Right now the matrix modn sparse class uses ints to store entries. The basic reason is the code

cdef struct c_vector_modint:
    int *entries

in sage/modules/vector_modn_sparse_h.pxi. This should probably instead use the unsigned C99 type uint_fast64_t.

CC: @sagetrac-Bouillaguet @ClementPernet @embray @videlec

Component: linear algebra

Author: Frédéric Chapoton

Branch/Commit: bc31936

Reviewer: Travis Scrimshaw

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

[vbraun]

comment:1

As discussed in #14627, this should really be signed int_fast64_t. Then it fits into a PyInt on all sensible (i.e. excluding Win64) platforms.

[fchapoton]

comment:7

tentative

---

New commits:

bc31936

using int_fast64 for vector_modn_sparse

[fchapoton]

Branch: u/chapoton/12679

[fchapoton]

Commit: bc31936

[fchapoton]

comment:9

Is this a good idea or not ?

[fchapoton]

Author: Frédéric Chapoton

[ClementPernet]

comment:10

I strongly support using ints with a specified size when they will be used for arithmetic. I am not familar with the Win64 support problem so I can not tell for this aspect, but otherwise, this seems like a very good move.

[fchapoton]

comment:11

Thanks. And the patchbot is green.

So we should ask Erik what happens on Windows. Erik ?

[fchapoton]

comment:12

Erik or anybody else ? review, opinion, please ?

[fchapoton]

comment:13

Do someone have any comments ? Could somebody please test on Windows ?

[tscrim]

Reviewer: Travis Scrimshaw

[tscrim]

comment:14

LGTM.

[embray]

comment:15

I do need to get the Cygwin patchbot working again. I was running one for a time but it went down, and I never got it running again. Samuel also had one running for a time but he's out of town and I don't know what happened to it. Maybe I'll work on that today.

Nevertheless I don't see any special reason to test this on Windows. int_fast_64_t is part of the C99 standard and will resolve to the appropriate type on the target platform.

[embray]

comment:16

Replying to @vbraun:

As discussed in #14627, this should really be signed int_fast64_t. Then it fits into a PyInt on all sensible (i.e. excluding Win64) platforms.

I see, you were worrying about this comment. But it's not an issue. I think the concern here was that on native Windows compilers sizeof(long) == 4 (and the Python 2 PyIntObject uses a long to store its value). But on Cygwin sizeof(long) == 8 so this is not a problem, as we don't support Sage on native Windows anyways.

Plus it's only an issue for Python 2. In the Python 3 PyLongObject which is used for all ints, it uses (by default, on any modern 64-bit system) exact bit size types.

[vbraun]

Changed branch from u/chapoton/12679 to bc31936