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: |
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:2 Changed 9 years ago by
Milestone: | sage-5.11 → sage-5.12 |
---|
comment:3 Changed 9 years ago by
Milestone: | sage-6.1 → sage-6.2 |
---|
comment:4 Changed 9 years ago by
Milestone: | sage-6.2 → sage-6.3 |
---|
comment:5 Changed 9 years ago by
Milestone: | sage-6.3 → sage-6.4 |
---|
comment:6 Changed 6 years ago by
Cc: | Bouillaguet added |
---|
comment:7 Changed 3 years ago by
Branch: | → u/chapoton/12679 |
---|---|
Commit: | → bc31936fb962b8078056e34aa91ab71c8f7e5692 |
Status: | new → needs_info |
comment:8 Changed 3 years ago by
Milestone: | sage-6.4 → sage-9.0 |
---|
comment:9 Changed 3 years ago by
Authors: | → Frédéric Chapoton |
---|---|
Cc: | cpernet added |
Status: | needs_info → needs_review |
Is this a good idea or not ?
comment:10 Changed 3 years ago by
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
Cc: | embray added |
---|
Thanks. And the patchbot is green.
So we should ask Erik what happens on Windows. Erik ?
comment:13 Changed 3 years ago by
Cc: | vdelecroix added |
---|
Do someone have any comments ? Could somebody please test on Windows ?
comment:14 Changed 3 years ago by
Reviewers: | → Travis Scrimshaw |
---|---|
Status: | needs_review → positive_review |
LGTM.
comment:15 Changed 3 years ago by
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 Changed 3 years ago by
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 int
s, it uses (by default, on any modern 64-bit system) exact bit size types.
comment:17 Changed 3 years ago by
Branch: | u/chapoton/12679 → bc31936fb962b8078056e34aa91ab71c8f7e5692 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
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.