#16742: Reimplementation of weak_popov_form for matrices
-------------------------------------+-------------------------------------
Reporter: ketzu | Owner:
Type: enhancement | Status: needs_review
Priority: minor | Milestone: sage-6.7
Component: performance | Resolution:
Keywords: matrix weak- | Merged in:
popov-form #16739 | Reviewers:
Authors: David Mödinger | Work issues:
Report Upstream: N/A | Commit:
Branch: | e464be9e38cd84c8d52d9c117724e4a9f0ce01e7
u/ketzu/overload_for_faster_weak_popov_form| Stopgaps:
Dependencies: #16888 |
-------------------------------------+-------------------------------------
Changes (by dlucas):
* cc: dlucas (added)
* status: new => needs_review
* milestone: sage-6.4 => sage-6.7
Comment:
Hello,
I'm working on the review for tickets related to the weal Popov form
problem. Even if this one was not on `needs_review` state, I considered
that it was quite old, and did not change for a while, so I took the
liberty to review it.
== General comments ==
g1) Several conflicts arise if merge with latest Sage beta. They need to
be fixed.
g2) There are some trailing whitespaces in the files you modified. It will
be nice to remove them.
g3) There are errors while building the documentation. To check if the
documentation builds as it should, and check if the html output looks good
or not, you can use `sage --docbuild reference/myFolder html`, replacing
my folder by the part of Sage you were working on. Here, it will be
`reference/matrices`.
== Documentation-related comments ==
d1) While writing documentation, you can use double backticks to have a
nice formatting for variables. If you use simple backticks, it will format
it as LaTeX test, which will just be italic if used on simple text. It is
advised in [http://www.sagemath.org/doc/developer/coding_basics.html
#documentation-strings Sage documentation] to use double backticks around
variables in the output block. Besides, it looks better for variables, or
Python-related keyworkds (like `True`, or `None`) to use this syntax.
d2) The title of the file `weak_popov.pyx` is splitted on the html
documentation. Write the title in one line, or use an underscore to avoid
this.
d3) After the merge with latest beta, especially with the content of
#16888, doctests of `weak_popov.pyx` are broken as `weak_popov_form`
method throws a deprecation warning.
d4) I think it's better to call directly the method you're doctesting. In
`mulders_storjohann`'s doctests, call `mulders_storjohann` instead of
`weak_popov`.
d5) On line 152 (examples for `mulders_storjohann`) in `weak_popov`, you
say "Generally matrices in weak popov form will just be returned". I think
they will always be returned. In that case, can you remove "generally"? If
I'm wrong, can you add an example in which we input a matrix in wPf which
is not returned as is?
d6) You can get a nice formatting on the `ALGORITHM` block
(`weak_popov.py:102`) if you use only one colon instead of two after the
`ALGORITHM` keyword.
d7) On both helper methods you wrote in `weak_popov.pyx`, you have this
block:
{{{
.. note::
This method is used in the mulders-storjohann algorithm.
}}}
I think it will be more explicit for the user to say something like "This
is a helper function for Mulders-Storjohann algorithm and should only be
used internally".
d8) At the beginning of each function, you write something like "Function
to do something". You can go straight to the point, and write directly
"Returns this" or "Computes that".
d9) In `simmple_transformation`'s doc, input parameter `U` is not
documented.
== Code and design related comments ==
c1) `mulders-storjohann` method takes a matrix `M` as an input, then
modifies it in place, and outputs the modified matrix. I think it's not an
intuitive behaviour. It should either modify the input in place, and
output nothing or copy the input and output the modified copy.
c2) With the merge of #16888, old `weak_popov_form` method will be
deprecated and renamed `row_reduced_form`. Which means that, without any
changes, one will need to call
`M.row_reduced_form(implementation="cython")` to get the weak Popov form
of `M`... Which is not intuitive at all. What I suggest here is to
reinstate the method `weak_popov_form` (as it will compute the wPf, we can
de-deprecate it) which will do the transformations to get the wPf of the
input using `mulders-storjohann` method. By doing that, we keep the
`row_reduced_form` method for users who absolutely want the row reduced
form of their matrices, and have a real weak Popov form computation with
methods explicitely named.
c3) As #16896 changes the calling conventions of `row_reduced_from`, I
think one the two tickets should depend on the other one to be sure we get
what we want in the end, namely a wPf method and a rrf method with
simplfied calling conventions.
--
Ticket URL: <http://trac.sagemath.org/ticket/16742#comment:19>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
