Opened 6 years ago

Closed 6 years ago

#17824 closed enhancement (fixed)

get rid of MatrixSpace_ZZ_2x2

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-6.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:

Status badges

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 6 years ago by vdelecroix

  • Branch set to u/vdelecroix/17824
  • Commit set to efb3d553d7c8c2daab2955f911507ce823c6bdb8
  • Status changed from new to needs_review

New commits:

f7e6496trac #17822: faster Matrix_integer_dense
1cb707btrac #17822: use mpz_fits_ulong_p
555bcd2trac #17822: TypeError -> ArithmeticError in _inverse_flint
2a3de63trac #17824: remove matrix_integer_2x2.pyx and .pxd
5622781trac #17824: deprecation and adaptation
efb3d55trac #17824: adapt sage.modular

comment:2 Changed 6 years ago by git

  • Commit changed from efb3d553d7c8c2daab2955f911507ce823c6bdb8 to 0a77864ed164b60025b2784d74c74f7b2c2ea19a

Branch pushed to git repo; I updated commit sha1. New commits:

0a77864trac #17824: fixed initialization

comment:3 Changed 6 years ago by git

  • Commit changed from 0a77864ed164b60025b2784d74c74f7b2c2ea19a to 511edd2fccfc97b4d94d26b616a74099d3079f59

Branch pushed to git repo; I updated commit sha1. New commits:

766f1a5Remove matrix_modn_dense
60b4314trac #17824: merge #17837
511edd2trac #17824: use unpickle_override

comment:4 Changed 6 years ago by vdelecroix

  • Dependencies changed from #17822 to #17822, #17837

comment:5 Changed 6 years ago by git

  • Commit changed from 511edd2fccfc97b4d94d26b616a74099d3079f59 to 9ef4c55e926530c13f4347aa9c12b8b47eb09b85

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

885673etrac #17824: remove matrix_integer_2x2
9ef4c55trac #17824: adapt sage.modular

comment:6 Changed 6 years ago by vdelecroix

Rebased on sage-6.6.beta2 in which the two dependencies are merged.

comment:7 Changed 6 years ago by jdemeyer

Can you replace PyList_CheckExact() by type(...) is list?

Also, why change bint -> int?

comment:8 Changed 6 years ago by jdemeyer

  • 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 6 years ago by jdemeyer

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 6 years ago by jdemeyer

I am making a review commit....

comment:11 Changed 6 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer

comment:12 Changed 6 years ago by jdemeyer

  • Branch changed from u/vdelecroix/17824 to u/jdemeyer/17824

comment:13 Changed 6 years ago by jdemeyer

  • 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:

8b75e98Further clean-up of 2x2 matrices and congruence subgroups

comment:14 follow-up: Changed 6 years ago by vdelecroix

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 6 years ago by jdemeyer

Replying to vdelecroix:

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.

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 6 years ago by vdelecroix

  • Authors changed from Vincent Delecroix to Vincent Delecroix, Jeroen Demeyer
  • 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 6 years ago by vbraun

  • Branch changed from u/jdemeyer/17824 to 8b75e9808285a5bed025ea8d66ec25837a1fca8e
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.