rework of row_reduced_form/weak_popov_form calling convention

[ketzu]

The calling convention of row_reduced_form and weak_popov_form is bizarre and should be fixed.

1. 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.

2. It returns a 3-tuple (W,U,d). Usually, W is the only object of interest (the row reduced form), while U is a transformation matrix which can sometimes be useful. d can be computed directly from W and shouldn't be returned at all.

3. 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>...

[ketzu]

New commits:

​4106d05

Deprecation include for keyword.

[git]

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.

[git]

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

​79bc1dd

Fixed doctests. Probably some new should be added.

[dlucas]

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.

[git]

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

​acb4f46	Deprecation include for keyword.
​65461d1	Fixed doctests. Probably some new should be added.
​dbcacc5	Merge error corrected.

[ketzu]

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.

[git]

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

​ce9c5ff	Corrected comments to row reduced form from weak popov form, added doctest using transformation=True, added parameter forwarding of transformation parameter.
​b54a732	Added clarification comment.

[ketzu]

I tried to incorporate the suggestions and create a patchable commit. Doctests and compilation/integration are working for me on sages git master branch.

[git]

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.

[jsrn]

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.

[dlucas]

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

[jsrn]

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...

[git]

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

​323e675	Fix return type when input type is a polynomial matrix
​12b7239	Drop dedent since it didn't have the intended purpose
​bf1732d	Fixed the base ring fix when input is over polynomial ring.

[git]

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

​67c5636

Ok, last fix

[jsrn]

Done!

[dlucas]

Ok, it passes the tests and fixes the bug I noticed, to me it's good to go.

[jsrn]

Cool, thanks!

[vbraun]

The reviewer name is supposed to be your real name, not trac account