Opened 7 years ago

Closed 5 years ago

Last modified 5 years ago

#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:

Status badges

Description (last modified by jsrn)

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

  • Authors set to David Mödinger
  • 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 ketzu

  • Dependencies set to #16888

comment:3 Changed 6 years ago by ketzu

  • Branch set to u/ketzu/rework_of_row_reduced_form_weak_popov_form_calling_convention

comment:4 Changed 6 years ago by ketzu

  • Commit set to 4106d05f9df82f5292e9aacb0aa4d0f3487e54f9
  • Status changed from new to needs_review

New commits:

4106d05Deprecation include for keyword.

comment:5 Changed 6 years ago by git

  • Commit changed from 4106d05f9df82f5292e9aacb0aa4d0f3487e54f9 to 3b827a5f14f8ad37abaa9e2ca62963f1b54e8e65

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

970fde5Rename 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.
12755f1Reduced to be a renaming only.
7820917Fixed the doctest of matrix/matrix_misc.py.
3b827a5merge from dependent ticket.

comment:6 Changed 6 years ago by git

  • Commit changed from 3b827a5f14f8ad37abaa9e2ca62963f1b54e8e65 to 79bc1dda27e09fea0fb168bd47e2805fab63e67b

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

79bc1ddFixed doctests. Probably some new should be added.

comment:7 Changed 6 years ago by dlucas

  • 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 dlucas

  • Cc dlucas added

comment:9 Changed 6 years ago by git

  • Commit changed from 79bc1dda27e09fea0fb168bd47e2805fab63e67b to dbcacc5ff086e8e85af8a9f9d99bac10f43bbae1

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

acb4f46Deprecation include for keyword.
65461d1Fixed doctests. Probably some new should be added.
dbcacc5Merge error corrected.

comment:10 Changed 6 years ago by 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.

comment:11 Changed 6 years ago by git

  • Commit changed from dbcacc5ff086e8e85af8a9f9d99bac10f43bbae1 to b54a7329a5f0baba05d17432e38de81afd4b291a

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

ce9c5ffCorrected comments to row reduced form from weak popov form, added doctest using transformation=True, added parameter forwarding of transformation parameter.
b54a732Added clarification comment.

comment:12 Changed 6 years ago by ketzu

  • 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 jsrn

  • 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 git

  • Commit changed from b54a7329a5f0baba05d17432e38de81afd4b291a to b4af64e3dca222d414c4906ce4e461a4015f4606

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

b4af64eReintroduced the old keywords and old calling convention for backward compatibility, but added deprecation warning.

comment:15 Changed 5 years ago by jsrn

  • Authors changed from David Mödinger to David Mödinger, Johan S. R. Nielsen
  • 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 dlucas

  • 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 jsrn

  • 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 git

  • Commit changed from b4af64e3dca222d414c4906ce4e461a4015f4606 to bf1732da8877ff2936c5fca4fe20ef7354cbd1fc

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

323e675Fix return type when input type is a polynomial matrix
12b7239Drop dedent since it didn't have the intended purpose
bf1732dFixed the base ring fix when input is over polynomial ring.

comment:19 Changed 5 years ago by git

  • Commit changed from bf1732da8877ff2936c5fca4fe20ef7354cbd1fc to 67c5636f25e30655a6cc5084f6e426fb181049ce

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

67c5636Ok, last fix

comment:20 Changed 5 years ago by jsrn

  • Description modified (diff)
  • Status changed from needs_work to needs_review

Done!

comment:21 Changed 5 years ago by dlucas

  • 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 jsrn

Cool, thanks!

comment:23 Changed 5 years ago by dlucas

  • Milestone changed from sage-6.7 to sage-6.10
  • Reviewers set to dlucas

comment:24 Changed 5 years ago by vbraun

  • 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 jsrn

  • Reviewers changed from dlucas to David Lucas
  • Status changed from needs_work to positive_review

comment:26 Changed 5 years ago by vbraun

  • 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 kcrisman

  • Authors changed from David Mödinger, Johan S. R. Nielsen to David Mödinger, Johan Sebastian Rosenkilde Nielsen
  • Commit 67c5636f25e30655a6cc5084f6e426fb181049ce deleted
Note: See TracTickets for help on using tickets.