Opened 6 years ago
Closed 6 years ago
#16888 closed defect (fixed)
weak popov form does not compute weak popov form
Reported by:  ketzu  Owned by:  

Priority:  major  Milestone:  sage6.6 
Component:  linear algebra  Keywords:  weakpopovform matrix 
Cc:  jsrn  Merged in:  
Authors:  David Mödinger  Reviewers:  David Lucas 
Report Upstream:  N/A  Work issues:  
Branch:  c19cc11 (Commits)  Commit:  c19cc1182669d5534f1371dd42c5ddbcdf7875d7 
Dependencies:  Stopgaps: 
Description (last modified by )
While working on the sage.matrix.matrix2.weak_popov_form (for sage.matrix.matrix_misc.weak_popov_form applies the same) method for performance issues I noticed something.
The weak Popov form as defined in [MS] is not computed by this method. The other references do not call this form weak Popov form, it is a les restrictive definition for a certain row reduced form of matrix.
Followup ticket for reimplementation of wpf: #16742 and #16896.
[MS] T. Mulders, A. Storjohann, "On lattice reduction for polynomial
matrices," J. Symbolic Comput. 35 (2003), no. 4, 377401
Comment of weak_popov_form:
OUTPUT:
A 3tuple `(W,N,d)` consisting of:
1. `W`  a matrix over `k(x)` giving a weak the Popov form of self
2. `N`  a matrix over `k[x]` representing row operations used to
transform `self` to `W`
3. `d`  degree of respective columns of W; the degree of a column is
the maximum of the degree of its elements
Change History (16)
comment:1 Changed 6 years ago by
 Cc jsrn added
 Component changed from PLEASE CHANGE to linear algebra
 Description modified (diff)
 Keywords weakpopovform matrix added
 Type changed from PLEASE CHANGE to defect
comment:2 Changed 6 years ago by
 Description modified (diff)
comment:3 Changed 6 years ago by
 Branch set to u/ketzu/weak_popov_form_does_not_compute_weak_popov_form
comment:4 Changed 6 years ago by
 Commit set to 970fde5ed4df264be62bc54c7b09e75e56643a08
comment:5 Changed 6 years ago by
 Commit changed from 970fde5ed4df264be62bc54c7b09e75e56643a08 to 12755f174d1775e53ebf719612f5e984d7120677
Branch pushed to git repo; I updated commit sha1. New commits:
12755f1  Reduced to be a renaming only.

comment:6 Changed 6 years ago by
 Description modified (diff)
comment:7 Changed 6 years ago by
See also #16896
comment:8 Changed 6 years ago by
 Status changed from new to needs_review
comment:9 Changed 6 years ago by
 Status changed from needs_review to needs_work
one doctest is failing
comment:10 Changed 6 years ago by
 Commit changed from 12755f174d1775e53ebf719612f5e984d7120677 to 78209179e32bead65ec5f3de09c343dc1d7ed297
Branch pushed to git repo; I updated commit sha1. New commits:
7820917  Fixed the doctest of matrix/matrix_misc.py.

comment:11 Changed 6 years ago by
 Status changed from needs_work to needs_review
Unfortunately I was sick for a long time, so i fixed the commit.
comment:12 Changed 6 years ago by
 Branch changed from u/ketzu/weak_popov_form_does_not_compute_weak_popov_form to u/dlucas/weak_popov_form_does_not_compute_weak_popov_form
comment:13 Changed 6 years ago by
 Commit changed from 78209179e32bead65ec5f3de09c343dc1d7ed297 to c19cc1182669d5534f1371dd42c5ddbcdf7875d7
I just changed some minor things into documentation : I removed any reference related to weak Popov form, because it does not compute the weak Popov form and I added a new reference related to the row reduced form. I also removed the name "weak Popov form" from the output field.
comment:14 Changed 6 years ago by
 Reviewers set to David Lucas
 Status changed from needs_review to positive_review
comment:15 Changed 6 years ago by
 Milestone changed from sage6.4 to sage6.6
comment:16 Changed 6 years ago by
 Branch changed from u/dlucas/weak_popov_form_does_not_compute_weak_popov_form to c19cc1182669d5534f1371dd42c5ddbcdf7875d7
 Resolution set to fixed
 Status changed from positive_review to closed
I think there should be a keyword ("transposition=False" oder something) to indicate the matrix N is wanted. Also I am not completely sure I did the deprecations right in any way.
New commits:
Rename of weak_popov_form() to row_reduced_form() added deprecation warning, rework of the use of the ascend parameter, removed sorting and returning d if ascend is not set.