Opened 11 years ago

Closed 3 years ago

#12679 closed enhancement (fixed)

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

Reported by: William Stein Owned by: jason, was
Priority: major Milestone: sage-9.0
Component: linear algebra Keywords:
Cc: Charles Bouillaguet, Clément Pernet, Erik Bray, Vincent Delecroix Merged in:
Authors: Frédéric Chapoton Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: bc31936 (Commits, GitHub, GitLab) Commit: bc31936fb962b8078056e34aa91ab71c8f7e5692
Dependencies: Stopgaps:

Status badges

Description

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.

Change History (17)

comment:1 Changed 10 years ago by Volker Braun

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.

Last edited 10 years ago by Volker Braun (previous) (diff)

comment:2 Changed 9 years ago by Jeroen Demeyer

Milestone: sage-5.11sage-5.12

comment:3 Changed 9 years ago by For batch modifications

Milestone: sage-6.1sage-6.2

comment:4 Changed 9 years ago by For batch modifications

Milestone: sage-6.2sage-6.3

comment:5 Changed 8 years ago by For batch modifications

Milestone: sage-6.3sage-6.4

comment:6 Changed 6 years ago by Charles Bouillaguet

Cc: Charles Bouillaguet added

comment:7 Changed 3 years ago by Frédéric Chapoton

Branch: u/chapoton/12679
Commit: bc31936fb962b8078056e34aa91ab71c8f7e5692
Status: newneeds_info

tentative


New commits:

bc31936using int_fast64 for vector_modn_sparse

comment:8 Changed 3 years ago by Frédéric Chapoton

Milestone: sage-6.4sage-9.0

comment:9 Changed 3 years ago by Frédéric Chapoton

Authors: Frédéric Chapoton
Cc: Clément Pernet added
Status: needs_infoneeds_review

Is this a good idea or not ?

comment:10 Changed 3 years ago by Clément Pernet

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.

comment:11 Changed 3 years ago by Frédéric Chapoton

Cc: Erik Bray added

Thanks. And the patchbot is green.

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

comment:12 Changed 3 years ago by Frédéric Chapoton

Erik or anybody else ? review, opinion, please ?

comment:13 Changed 3 years ago by Frédéric Chapoton

Cc: Vincent Delecroix added

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

comment:14 Changed 3 years ago by Travis Scrimshaw

Reviewers: Travis Scrimshaw
Status: needs_reviewpositive_review

LGTM.

comment:15 Changed 3 years ago by Erik Bray

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.

comment:16 in reply to:  1 Changed 3 years ago by Erik Bray

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.

comment:17 Changed 3 years ago by Volker Braun

Branch: u/chapoton/12679bc31936fb962b8078056e34aa91ab71c8f7e5692
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.