#11528: Allow deleting row or column from matrix
------------------------------+---------------------------------------------
Reporter: kcrisman | Owner: jason, was
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-5.0
Component: linear algebra | Keywords: delete remove
Work_issues: | Upstream: N/A
Reviewer: | Author:
Merged: | Dependencies:
------------------------------+---------------------------------------------
Changes (by rbeezer):
* status: needs_review => needs_work
Comment:
Pong,
This looks like a great contribution. Lots of small suggestions or
changes to match Sage style or practice. In no particular order.
Rob
1. You might just rename to main arguments `cols` and `rows` for
simplicity. The deletion part should be apparent from the function name.
1. I think the convention in Sage is to default to doing checks, and
let a user explicitly ask to turn them off to gain speed, or when they
know the routine is being fed good input. So I would switch `index_check`
to `True`. You might look around, I think just `check` is used quite
frequently. List the default in the docstring right after the name of the
variable.
1. Your check does not check to see if the list or tuple actual
contains integers. So
{{{
sage: A.delete_columns(['blue'], index_check=True)
<snip>
IndexError: column indices in ['blue'] are out of range.
}}}
gives an error message that is not totally accurate.
1. I wonder if this would be faster if sets are not used, especially
when you are trying to use Cython improvements with cdef variables. What
happens if you were to form cols with an actual loop, or maybe a list
comprehension like
{{{
cols = [x for x in range(self.ncols()) if not x in cols_to_delete]
}}}
You can easily see generated C code in the notebook with %cython as the
first line of a cell, then click on the generated links.
1. You should use the new Python 3.0 syntax for errors (`raise
IndexError("explanation")` if I remember right), and `.format()` for
formatted strings.
1. In documentation always use {{{``}}} on both ends of all uses of
variable names (and not regular quote marks). Indent the bit with your
name in the AUTHOR block and it will end up looking better. Put a space
before your double-colon verbatim indicators. Right now one survives in
the text. Otherwise, documentation looks good.
1. Add an "Apply" section to the summary, with a link to the patch.
See any recently merged ticket for style and markup. Release manager will
require it.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/11528#comment:8>
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.
