Opened 7 years ago
Closed 7 years ago
#17824 closed enhancement (fixed)
get rid of MatrixSpace_ZZ_2x2
Reported by:  vdelecroix  Owned by:  

Priority:  major  Milestone:  sage6.6 
Component:  linear algebra  Keywords:  
Cc:  Merged in:  
Authors:  Vincent Delecroix, Jeroen Demeyer  Reviewers:  Jeroen Demeyer, Vincent Delecroix 
Report Upstream:  N/A  Work issues:  
Branch:  8b75e98 (Commits, GitHub, GitLab)  Commit:  8b75e9808285a5bed025ea8d66ec25837a1fca8e 
Dependencies:  #17822, #17837  Stopgaps: 
Description
With #17822 the timings of the generic integer matrix class in sage.matrix.matrix_integer_dense
(using flint) are identical to the custom ones in sage.matrix.matrix_integer_2x2
. With this ticket, we get rid of the latter.
Change History (17)
comment:1 Changed 7 years ago by
 Branch set to u/vdelecroix/17824
 Commit set to efb3d553d7c8c2daab2955f911507ce823c6bdb8
 Status changed from new to needs_review
comment:2 Changed 7 years ago by
 Commit changed from efb3d553d7c8c2daab2955f911507ce823c6bdb8 to 0a77864ed164b60025b2784d74c74f7b2c2ea19a
Branch pushed to git repo; I updated commit sha1. New commits:
0a77864  trac #17824: fixed initialization

comment:3 Changed 7 years ago by
 Commit changed from 0a77864ed164b60025b2784d74c74f7b2c2ea19a to 511edd2fccfc97b4d94d26b616a74099d3079f59
comment:4 Changed 7 years ago by
 Dependencies changed from #17822 to #17822, #17837
comment:5 Changed 7 years ago by
 Commit changed from 511edd2fccfc97b4d94d26b616a74099d3079f59 to 9ef4c55e926530c13f4347aa9c12b8b47eb09b85
comment:6 Changed 7 years ago by
Rebased on sage6.6.beta2 in which the two dependencies are merged.
comment:7 Changed 7 years ago by
Can you replace PyList_CheckExact()
by type(...) is list
?
Also, why change bint
> int
?
comment:8 Changed 7 years ago by
 Status changed from needs_review to needs_work
_invert_flint
always returns a positive integer as second component (even though this isn't documented!), so the case 1
in _invert_unit()
cannot occur.
comment:9 Changed 7 years ago by
For the PyList_CheckExact()
check: I also dislike the double negation in the else
clause: change the last elif not
to else
and change the else
to elif
.
comment:10 Changed 7 years ago by
I am making a review commit....
comment:11 Changed 7 years ago by
 Reviewers set to Jeroen Demeyer
comment:12 Changed 7 years ago by
 Branch changed from u/vdelecroix/17824 to u/jdemeyer/17824
comment:13 Changed 7 years ago by
 Commit changed from 9ef4c55e926530c13f4347aa9c12b8b47eb09b85 to 8b75e9808285a5bed025ea8d66ec25837a1fca8e
 Status changed from needs_work to needs_review
I ended up doing some more substantial changes, such as renaming congroup_pyx.pyx
> congroup.pyx
, I hope you don't mind.
New commits:
8b75e98  Further cleanup of 2x2 matrices and congruence subgroups

comment:14 followup: ↓ 15 Changed 7 years ago by
As far as I understand, the Cython bint
type corresponds to to a C int
that will be converted to Python bool
when needed. In the present case, is_list
is never used as a Python value so I do not see the point of making it a bint
.
comment:15 in reply to: ↑ 14 Changed 7 years ago by
Replying to vdelecroix:
As far as I understand, the Cython
bint
type corresponds to to a Cint
that will be converted to Pythonbool
when needed. In the present case,is_list
is never used as a Python value so I do not see the point of making it abint
.
For exactly the same reason, I do not see the point of making it an int
.
Using bint
is more explicit about what the variable is used for.
comment:16 Changed 7 years ago by
 Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Vincent Delecroix
 Status changed from needs_review to positive_review
All right. Let it go.
Vincent
comment:17 Changed 7 years ago by
 Branch changed from u/jdemeyer/17824 to 8b75e9808285a5bed025ea8d66ec25837a1fca8e
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
trac #17822: faster Matrix_integer_dense
trac #17822: use mpz_fits_ulong_p
trac #17822: TypeError > ArithmeticError in _inverse_flint
trac #17824: remove matrix_integer_2x2.pyx and .pxd
trac #17824: deprecation and adaptation
trac #17824: adapt sage.modular