#6441: [with patch, needs review] Charpoly (plus adjoint and det)
-----------------------+----------------------------------------------------
Reporter: spancratz | Owner: somebody
Type: defect | Status: new
Priority: minor | Milestone: sage-4.1
Component: algebra | Keywords: charpoly, division-free
Reviewer: | Author: Sebastian Pancratz
Merged: |
-----------------------+----------------------------------------------------
Comment(by rbeezer):
Sebastian,
Having these algorithms in Sage will a great addition. Any chance you
have an electronic copy of [Se02] you might be able to send me (at
[email protected])? It'd make it easier to review the main chunks of new
code.
On a first pass, I've noticed a few things with Sage style and procedures
that need attention. Not the fun stuff, but required to get code
included. In no real order.
1. In docstrings, lists don't need blank lines to split them, and
sublists (like the choices for "algorithm" options) should be indented
(ala Python code) to make them render properly.
2. Constructions like ``"hessenberg"`` would not usually include the
inner quotes, I think, though I can see the argument for including them.
In any event, I can't recall seeing them before. The back-quotes will
cause different formatting anyway.
3. There was just a discussion on sage-devel about style on error
messages. No period on the end. ("self must be a square matrix." for one
{{{ArithemticError}}}) I've been inclined to use {{{ValueError}}} in a
case like a matrix of the wrong shape.
4. In docstrings the :: always begins a verbatim block, but it is "smart"
and behaves 3 ways.
(a) On a line all by itself, then doesn't appear in output.[[BR]]
(b) At end of a sentence with no space, then output as a single semi-
colon (like on TESTS::).[[BR]]
(c) At end of sentence, with space between, then no output.[[BR]]
So you have several places where you can replace a couple of blank lines
and a :: line by just using placing :: after a preceding sentence with
space.
5. No need to write INPUT:: if there isn't any. I saw one "Tests" that
needed capitalization.
6. A big one - every function needs a doctest, even underscore functions.
You can meet the letter of the law, or work hard to make tests that will
expose broken code when others "improve" your work or other places. For
example, {{{_is_certainly_integral_domain}}} needs doctests (which in this
particular case might test a case that is True and a case that is False).
7. I like lots of author's names in the files, but they also migrate to
the reference manual, where minor changes don't really need reporting and
I think clutter up the manual. I like the names in the source so I know
who to go to with questions. So maybe go for a bit more balance and *do*
include your name on the new big routines and the places where they have a
big impact, and perhaps not in places where minor changes are needed in
doctests. In theory, the Mercurial log and Trac can be used to chase down
the origin of minor changes.
8. {{{sage -docbuild reference html}}}[[BR]]
will rebuild the HTML reference manual. First time takes a while, but
subsequent uses just update changed files. This is a good way to see what
the docstrings become (and something any reviewer should be checking).
9. {{{sage -coverage <file>}}}[[BR]]
will give you a doctest coverage report - again, something a reviewer will
be checking.
I'll continue looking at the code, but I thought I'd pass these along
right away. Since this is your first patch, I hope you don't mind all the
advice on the mundane (yet important) stuff. This looks like a very solid
first contribution, and as I said above, will be very welcome in Sage.
Rob
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/6441#comment:3>
Sage <http://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
-~----------~----~----~----~------~----~------~--~---
