#10347: Implementation of is_(skew_)symmetrizable for matrices
-----------------------------+----------------------------------------------
Reporter: stumpc5 | Owner: tbd
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-4.7.1
Component: combinatorics | Keywords: symmetrizable matrix
Work_issues: | Upstream: N/A
Reviewer: Hugh Thomas | Author: Christian Stump
Merged: | Dependencies:
-----------------------------+----------------------------------------------
Comment(by hthomas):
Hi Christian!
Below are all my comments. The first one is something which should be
fixed (but is trivial). For the others, I don't know if any change needs
to be made. Suggestions occurred to me, but perhaps you had good reasons
for doing things as you did, and in any case, it doesn't seem to me that
these are likely to be routines that are time-critical. I am perhaps
over-thinking this process, because of its being the first time I am
reviewing a patch.
* The description of the return value for is_skew_symmetrizable,
is_skew_symmetric, and _check_symmetrizability when return_diag is true
says that it returns the diagonal matrix, when in fact, it returns the
diagonal of the diagonal matrix. (I like the output as is, I just think
the documentation should change.)
* In _travel_column, technically, I think it makes more sense to check
that i isn't in dict before getting self_ik and self_ki, and testing
whether or not bool(self_ik) and bool(self_ki) agree -- if i is in dict,
this has already been checked.
* When return_diag is set, I don't see the point of saying [ d[i] for i
in sorted( d.keys() ) ]. Why not just say [ d[i] for i in range(
self._ncols ) ]?
* If the entries in the input matrix aren't in an integral domain, things
will get ugly. I realize that's far from the intended use case. What's
the protocol for something like that? Maybe just that the docstring
should say that the input is a matrix in an integral domain? Or is it
even necessary to do anything at all?
* I don't really like the following, but, again, it's far from the
intended use case (and it's not the fault of the code here):
{{{
sage: R.<x>=PolynomialRing(QQ)
sage: M=matrix([[1,x],[-x,1]])
sage: M.is_symmetrizable(positive=true)
True
sage: M.is_symmetrizable(positive=true, return_diag=true)
[1, -1]
}}}
I am happy to give a positive review as soon as you fix at least the first
point above (assuming the tests, which are running on my computer right
now, pass). Or should I wait until the patchbot is also happy? Unless
you think someone else should also review it, since I'm pretty
inexperienced.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/10347#comment:11>
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 post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
