#16896 closed enhancement (fixed)
rework of row_reduced_form/weak_popov_form calling convention
Reported by: | ketzu | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.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 3-tuple
(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 weak-popov-form 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 sage-6.4 to sage-6.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; weak-popov-form 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 deprecation-dance...
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 sage-6.7 to sage-6.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.