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: was Owned by: jason, was
Priority: major Milestone: sage-9.0
Component: linear algebra Keywords:
Cc: Bouillaguet, cpernet, embray, vdelecroix 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:

GitHub link to the corresponding issue

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 vbraun

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

Version 0, edited 10 years ago by vbraun (next)

comment:2 Changed 9 years ago by jdemeyer

Milestone: sage-5.11sage-5.12

comment:3 Changed 9 years ago by vbraun_spam

Milestone: sage-6.1sage-6.2

comment:4 Changed 9 years ago by vbraun_spam

Milestone: sage-6.2sage-6.3

comment:5 Changed 9 years ago by vbraun_spam

Milestone: sage-6.3sage-6.4

comment:6 Changed 6 years ago by Bouillaguet

Cc: Bouillaguet added

comment:7 Changed 3 years ago by 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 chapoton

Milestone: sage-6.4sage-9.0

comment:9 Changed 3 years ago by chapoton

Authors: Frédéric Chapoton
Cc: cpernet added
Status: needs_infoneeds_review

Is this a good idea or not ?

comment:10 Changed 3 years ago by cpernet

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 chapoton

Cc: embray added

Thanks. And the patchbot is green.

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

comment:12 Changed 3 years ago by chapoton

Erik or anybody else ? review, opinion, please ?

comment:13 Changed 3 years ago by chapoton

Cc: vdelecroix added

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

comment:14 Changed 3 years ago by tscrim

Reviewers: Travis Scrimshaw
Status: needs_reviewpositive_review

LGTM.

comment:15 Changed 3 years ago by embray

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 embray

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 vbraun

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