Opened 4 years ago
Last modified 2 years ago
#23486 needs_work enhancement
Echelon form for padic matrices
Reported by:  caruso  Owned by:  

Priority:  major  Milestone:  sage8.1 
Component:  padics  Keywords:  sd87, padicIMA, padicBordeaux 
Cc:  roed, saraedum, TristanVaccon  Merged in:  
Authors:  Xavier Caruso  Reviewers:  Ben Hutz 
Report Upstream:  N/A  Work issues:  
Branch:  u/roed/padic_hermite (Commits, GitHub, GitLab)  Commit:  e522c94b89719bc51d7069cf0b682968814ff449 
Dependencies:  #23450  Stopgaps: 
Description
This ticket implements echelon form for matrices over complete discrete valuation rings/fields.
Change History (20)
comment:1 Changed 4 years ago by
 Branch set to u/caruso/padic_hermite
comment:2 Changed 4 years ago by
 Commit set to 013880536ee5ec0499dcd749d58f3b36ec6b71cd
 Dependencies set to #23450
comment:3 Changed 4 years ago by
 Commit changed from 013880536ee5ec0499dcd749d58f3b36ec6b71cd to 02aa72a42558cad734be80d626af5552bd44751a
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
1f731ee  Check correctly (hopefully) that precision on input is enough

1f07da5  Small fixes

e265c77  Merge branch 'padic_smith' into padic_hermite

0c3e7cc  Fix precision issues

55b99df  Small fix in lift_to_maximal_precision

1c396e8  Merge branch 'padic_smith' into padic_hermite

71c0c74  Add doctest

731fb19  Fix doctest

1ea48ac  Two small bugs fixed

02aa72a  Merge branch 'padic_smith' into padic_hermite

comment:4 Changed 4 years ago by
Ticket ready for review!
comment:5 Changed 4 years ago by
 Status changed from new to needs_review
comment:6 Changed 4 years ago by
 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 warnlong 72.0 rings/laurent_series_ring.py # 1 doctest failed sage t warnlong 72.0 rings/padics/tests.py # 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
else: 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 4 years ago by
 Commit changed from 02aa72a42558cad734be80d626af5552bd44751a to ba4a937c4a44536da0273d736000619476707206
comment:8 Changed 4 years ago by
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 rowechelon 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 4 years ago by
 Status changed from needs_work to needs_review
comment:10 Changed 4 years ago by
 Branch changed from u/caruso/padic_hermite to u/roed/padic_hermite
comment:11 Changed 3 years ago by
 Commit changed from ba4a937c4a44536da0273d736000619476707206 to 190ba1bf1b68eb012e7b42900a7fd50096860b07
comment:12 Changed 3 years ago by
Great! So, what do we do with this ticket (which is now a kind of redundant)?
comment:13 Changed 3 years ago by
Are there any features here that aren't included in #17272?
comment:14 Changed 3 years ago by
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 3 years ago by
 Keywords padicIMA added
comment:16 Changed 3 years ago by
 Branch changed from u/roed/padic_hermite to u/caruso/padic_hermite
comment:17 Changed 3 years ago by
 Commit changed from 190ba1bf1b68eb012e7b42900a7fd50096860b07 to 8bf47ee19f44ee40782082fcbff5c09b76e66d17
 Status changed from needs_review to needs_work
New commits:
8bf47ee  Merge branch '8.3.rc1' into t/23486/padic_hermite

comment:18 Changed 2 years ago by
 Keywords padicBordeaux added
comment:19 Changed 2 years ago by
 Branch changed from u/caruso/padic_hermite to u/roed/padic_hermite
comment:20 Changed 2 years ago by
 Commit changed from 8bf47ee19f44ee40782082fcbff5c09b76e66d17 to e522c94b89719bc51d7069cf0b682968814ff449
I merged in the latest beta, but this still needs to be reconciled with #17272. I think there are features here that we want that weren't done by #17272 (for example, when the base ring is Zp), so this ticket should remain open.
New commits:
e522c94  Merge branch 'u/caruso/padic_hermite' of git://trac.sagemath.org/sage into 23486_padic_hermite

New commits:
Smith form of a padic matrix
Code moved into the category
Factorisation of code
Small fix
Move relevant code to sage.matrix.matrix_cdv_dense
Fix import
Add method tracks_precision()
Echelon form for matrices over CDVR/CDVF