Opened 2 years ago

Last modified 12 months ago

#23486 needs_work enhancement

Echelon form for p-adic matrices

Reported by: caruso Owned by:
Priority: major Milestone: sage-8.1
Component: padics Keywords: sd87, padicIMA
Cc: roed, saraedum, TristanVaccon Merged in:
Authors: Xavier Caruso Reviewers: Ben Hutz
Report Upstream: N/A Work issues:
Branch: u/caruso/padic_hermite (Commits) Commit: 8bf47ee19f44ee40782082fcbff5c09b76e66d17
Dependencies: #23450 Stopgaps:


This ticket implements echelon form for matrices over complete discrete valuation rings/fields.

Change History (17)

comment:1 Changed 2 years ago by caruso

  • Branch set to u/caruso/padic_hermite

comment:2 Changed 2 years ago by caruso

  • Commit set to 013880536ee5ec0499dcd749d58f3b36ec6b71cd
  • Dependencies set to #23450

New commits:

879f4ebSmith form of a p-adic matrix
595bc11Code moved into the category
1832757Factorisation of code
e2cdd99Small fix
7ae1018Move relevant code to sage.matrix.matrix_cdv_dense
7e73fd2Fix import
1fd67feAdd method tracks_precision()
0138805Echelon form for matrices over CDVR/CDVF

comment:3 Changed 2 years ago by git

  • Commit changed from 013880536ee5ec0499dcd749d58f3b36ec6b71cd to 02aa72a42558cad734be80d626af5552bd44751a

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

1f731eeCheck correctly (hopefully) that precision on input is enough
1f07da5Small fixes
e265c77Merge branch 'padic_smith' into padic_hermite
0c3e7ccFix precision issues
55b99dfSmall fix in lift_to_maximal_precision
1c396e8Merge branch 'padic_smith' into padic_hermite
71c0c74Add doctest
731fb19Fix doctest
1ea48acTwo small bugs fixed
02aa72aMerge branch 'padic_smith' into padic_hermite

comment:4 Changed 2 years ago by caruso

Ticket ready for review!

comment:5 Changed 2 years ago by caruso

  • Status changed from new to needs_review

comment:6 Changed 2 years ago by bhutz

  • Reviewers set to Ben Hutz
  • Status changed from needs_review to needs_work

I looked at this from a functionality perspective as opposed to looking through the algorithm line by line.

All of the examples I was able to come up with behaved as expected, so I just have a few minor issues

  • 2 doc test failures
    sage -t --warn-long 72.0 rings/  # 1 doctest failed
    sage -t --warn-long 72.0 rings/padics/  # 1 doctest failed
  • in a couple places you say
    if `d_i` denotes the `(i,i)` entry of `D`

I think you means D to be S

  • extra word
    'it is possible to do not'
  • in several places you say 'also possible for rectangular matrices' where you mean 'nonrectangular'
  • in two different places for echelon form you state it as find the Smith normal form instead of the echelon form
    to determine the Smith normal form
  • is there a reason to assign
            left = right = None

they don't seem to be used later.

  • incorrect word
integer this of this

should be 'integer ring of this'

comment:7 Changed 2 years ago by git

  • Commit changed from 02aa72a42558cad734be80d626af5552bd44751a to ba4a937c4a44536da0273d736000619476707206

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

52a4344Added keyword secure
65ffbc6Use the name "integral_smith_form" for matrices over CDVF
f27820aSmall fixes
ba4a937Merge branch 'padic_smith' into padic_hermite

comment:8 Changed 2 years ago by caruso

Thanks for your comments.

I fixed the wording. The failing doctests were due to the fact that, in the case of a matrix M defined over a complete discrete valuation *field* K, my function echelon_form() did not compute the row-echelon from of M over K but over its ring of integers (i.e. the transformation matrix has coefficients in the ring of integers and is invertible over it). I then changed the name for integral_echelon_from().

NB: Some of your comments are related to ticket #23450. Maybe you should look at this one as well.

comment:9 Changed 2 years ago by caruso

  • Status changed from needs_work to needs_review

comment:10 Changed 17 months ago by roed

  • Branch changed from u/caruso/padic_hermite to u/roed/padic_hermite

comment:11 Changed 14 months ago by roed

  • Commit changed from ba4a937c4a44536da0273d736000619476707206 to 190ba1bf1b68eb012e7b42900a7fd50096860b07

See also #17272, which just got a positive review.

New commits:

190ba1bMerge branch 'u/caruso/padic_hermite' of git:// into t/23486/padic_echelon

comment:12 Changed 14 months ago by caruso

Great! So, what do we do with this ticket (which is now a kind of redundant)?

comment:13 Changed 14 months ago by roed

Are there any features here that aren't included in #17272?

comment:14 Changed 14 months ago by caruso

The computation of transformation matrices, I think.

Moreover, for matrices over Qp, it makes sense to have an option for computing the echelon form over Zp (that is, without renormalizing the pivots) in order to save precision. But I don't think that it's implemented in this ticket.

comment:15 Changed 12 months ago by roed

  • Keywords padicIMA added

comment:16 Changed 12 months ago by caruso

  • Branch changed from u/roed/padic_hermite to u/caruso/padic_hermite

comment:17 Changed 12 months ago by caruso

  • Commit changed from 190ba1bf1b68eb012e7b42900a7fd50096860b07 to 8bf47ee19f44ee40782082fcbff5c09b76e66d17
  • Status changed from needs_review to needs_work

New commits:

8bf47eeMerge branch '8.3.rc1' into t/23486/padic_hermite
Note: See TracTickets for help on using tickets.