#16896 closed enhancement (fixed)
rework of row_reduced_form/weak_popov_form calling convention
Reported by:  ketzu  Owned by:  

Priority:  major  Milestone:  sage6.10 
Component:  linear algebra  Keywords:  polynomial matrix 
Cc:  jsrn, dlucas  Merged in:  
Authors:  David Mödinger, Johan Sebastian Rosenkilde Nielsen  Reviewers:  David Lucas 
Report Upstream:  N/A  Work issues:  
Branch:  67c5636 (Commits, GitHub, GitLab)  Commit:  
Dependencies:  #16888  Stopgaps: 
Description (last modified by )
The calling convention of row_reduced_form
and weak_popov_form
is bizarre and should be fixed.
 It takes a parameter
ascend
but this is ignored. It's original purpose was something like sorting the rows, but this is not part of weak Popov form or row reduced, and shouldn't pollute this method.
 It returns a 3tuple
(W,U,d)
. Usually,W
is the only object of interest (the row reduced form), whileU
is a transformation matrix which can sometimes be useful.d
can be computed directly fromW
and shouldn't be returned at all.
 When the input is a matrix over a polynomial ring, it returns the output as over the fraction field, though the output always *lie* in the polynomial ring. This is senseless and confusing.
I've added optional parameters transformation = False
for returning U
. To enable printing a deprecation warning for at least a year while supporting the old call style, I've added optional parameter old_call = True
. Set this to False
to get the future call style and disable deprecation warning. In one year, we'll change the default behaviour but retain old_call
as an acceptable argument, but print a deprecation warning if one sets it. In two years, we can finally remove old_call
and have the clean calling convention. <sigh>...
Change History (27)
comment:1 Changed 7 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 enhancement
comment:2 Changed 7 years ago by
 Dependencies set to #16888
comment:3 Changed 6 years ago by
 Branch set to u/ketzu/rework_of_row_reduced_form_weak_popov_form_calling_convention
comment:4 Changed 6 years ago by
 Commit set to 4106d05f9df82f5292e9aacb0aa4d0f3487e54f9
 Status changed from new to needs_review
comment:5 Changed 6 years ago by
 Commit changed from 4106d05f9df82f5292e9aacb0aa4d0f3487e54f9 to 3b827a5f14f8ad37abaa9e2ca62963f1b54e8e65
Branch pushed to git repo; I updated commit sha1. New commits:
970fde5  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.

12755f1  Reduced to be a renaming only.

7820917  Fixed the doctest of matrix/matrix_misc.py.

3b827a5  merge from dependent ticket.

comment:6 Changed 6 years ago by
 Commit changed from 3b827a5f14f8ad37abaa9e2ca62963f1b54e8e65 to 79bc1dda27e09fea0fb168bd47e2805fab63e67b
Branch pushed to git repo; I updated commit sha1. New commits:
79bc1dd  Fixed doctests. Probably some new should be added.

comment:7 Changed 6 years ago by
 Milestone changed from sage6.4 to sage6.7
 Status changed from needs_review to needs_work
Hello,
A few comments on this ticket:
1) Merge with latest beta of Sage is not possible as is, as some conflicts arise with the merge. It needs to be fixed.
2) There are some trailing whitespaces in both matrix2.pyx
and matrix_misc.py
, it will be good to remove them.
3) The documentation of row_reduced_form
mentions weak Popov form several times, like:
OUTPUT: `W`  a matrix over `k(x)` giving a weak the Popov form of self
but as far as I understand it, row_reduced_form
does not compute the weak Popov form, for instance:
sage: R.<t> = QQ['t'] sage: M = matrix([[t,t,t^2],[0,0,t], [0, t, 0]]) sage: M.row_reduced_form() [t t 0] [0 0 t] [0 t 0]
does not output a matrix in wPf as leading positions are the same for the first and third third rows of M
. In that case, change occurences of weak Popov form by row reduced form.
4) It could be nice to add another example in matrix_misc.py
to illustrate the output in the case where transformation=True
.
5) You changed the arguments of row_reduced_form
in this ticket. But in #16742, you will get the original implementation of row_reduced_form
, which might create a conflict between the two tickets. You need to add a dependency somewhere to be sure that the merge goes smoothly.
comment:8 Changed 6 years ago by
 Cc dlucas added
comment:9 Changed 6 years ago by
 Commit changed from 79bc1dda27e09fea0fb168bd47e2805fab63e67b to dbcacc5ff086e8e85af8a9f9d99bac10f43bbae1
comment:10 Changed 6 years ago by
Tried to rebase the commits for 6.8beta or at least 6.7 and solve all conflicts. Rework of comments including the term "weak Popov form" are yet to be done and testing/doctesting of current changes.
comment:11 Changed 6 years ago by
 Commit changed from dbcacc5ff086e8e85af8a9f9d99bac10f43bbae1 to b54a7329a5f0baba05d17432e38de81afd4b291a
comment:12 Changed 6 years ago by
 Status changed from needs_work to needs_review
I tried to incorporate the suggestions and create a patchable commit. Doctests and compilation/integration are working for me on sages git master branch.
comment:13 Changed 5 years ago by
 Branch changed from u/ketzu/rework_of_row_reduced_form_weak_popov_form_calling_convention to u/jsrn/rework_of_row_reduced_form_weak_popov_form_calling_convention
comment:14 Changed 5 years ago by
 Commit changed from b54a7329a5f0baba05d17432e38de81afd4b291a to b4af64e3dca222d414c4906ce4e461a4015f4606
Branch pushed to git repo; I updated commit sha1. New commits:
b4af64e  Reintroduced the old keywords and old calling convention for backward compatibility, but added deprecation warning.

comment:15 Changed 5 years ago by
 Description modified (diff)
 Keywords polynomial added; weakpopovform removed
I rewrote the patch because it didn't really fix the calling convention, while it still broke backwards compatibility without proper deprecation warnings. Now, the patch does as the new description says.
It's again at needs review.
comment:16 Changed 5 years ago by
 Status changed from needs_review to needs_work
I guess I'm bad news' harbinger again, as I noticed the following:
sage: PR.<x> = GF(7)[] sage: M = random_matrix(PR, 5, 5) sage: type(M) <type 'sage.rings.polynomial.polynomial_zmod_flint.Polynomial_zmod_flint'> sage: type(M.row_reduced_form(transformation=False, old_call=False)[0, 0]) <type 'sage.rings.fraction_field_FpT.FpTElement'>
So it seems that the actual implementation messes with the type of the elements in the matrix, which is never a good thing...
David
comment:17 Changed 5 years ago by
 Description modified (diff)
That has been the behaviour of the function always, so it's not the "fault" of this patch. It is, however, rather stupid, as it is because of the division with R.one()
(named den
) in the return line of row_reduced_form
in matrix_misc.py
.
So, let's say it was broken behaviour before, so we can change it without doing the deprecationdance...
comment:18 Changed 5 years ago by
 Commit changed from b4af64e3dca222d414c4906ce4e461a4015f4606 to bf1732da8877ff2936c5fca4fe20ef7354cbd1fc
comment:19 Changed 5 years ago by
 Commit changed from bf1732da8877ff2936c5fca4fe20ef7354cbd1fc to 67c5636f25e30655a6cc5084f6e426fb181049ce
Branch pushed to git repo; I updated commit sha1. New commits:
67c5636  Ok, last fix

comment:20 Changed 5 years ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
Done!
comment:21 Changed 5 years ago by
 Status changed from needs_review to positive_review
Ok, it passes the tests and fixes the bug I noticed, to me it's good to go.
comment:22 Changed 5 years ago by
Cool, thanks!
comment:23 Changed 5 years ago by
 Milestone changed from sage6.7 to sage6.10
 Reviewers set to dlucas
comment:24 Changed 5 years ago by
 Status changed from positive_review to needs_work
The reviewer name is supposed to be your real name, not trac account
comment:25 Changed 5 years ago by
 Reviewers changed from dlucas to David Lucas
 Status changed from needs_work to positive_review
comment:26 Changed 5 years ago by
 Branch changed from u/jsrn/rework_of_row_reduced_form_weak_popov_form_calling_convention to 67c5636f25e30655a6cc5084f6e426fb181049ce
 Resolution set to fixed
 Status changed from positive_review to closed
comment:27 Changed 5 years ago by
 Commit 67c5636f25e30655a6cc5084f6e426fb181049ce deleted
New commits:
Deprecation include for keyword.