Allow deleting row or column from matrix

[kcrisman]

This was requested in the 2011 Sage MAA PREP workshop. Names deleted.

: Is there any way to delete specified rows or columns from an existing matrix?
: Hmm, I think that is possible, using a method.
: I'll look.
:  - we can swap rows, scale rows, add mults of rows, create a matrix from specified rows... but not that.
: Would you find this useful?  Is it in Matlab?
: For list there is  a method called pop
: Right, but not for matrices.
: if matrix is a list of list...
: But it's not quite that.
: I'l make a ticket.
: this would be useful
: there is an m.insert_row
: Hah!  But we can't do it backwards   

So let's make this happen.

Apply:

1. trac_11528_delete_rows_and_columns.path.patch
2. trac_11528_matrix_delete_rows_and_columns.patch
3. trac_11528_matrix_delete_rows_columns.patch
4. trac_11528-reviewer.patch

[rbeezer]

You could implement this with a simple call to matrix_from_columns and friends.

[pong]

I was looking at the same issue last night. And Rob's suggestion does the job. Should we go ahead and closed the ticket?

[kcrisman]

I think Rob is saying that this could be implemented, not that we shouldn't add a new command. So there is something to do. You want to create it?

[pong]

Hum... sure. For example, something like

def delete_col(A,col_to_del):

cols = list(set(range(A.ncols()))-set(col_to_del)); return A.matrix_from_columns(cols)

will work. I just need to need to recall how to turn that into a method for matrix. Any pointer for that? Also, do we want indexing the columns starting from 1 or 0? (the implementation above assume the index starts from 0).

[kcrisman]

Yeah, sure. Look in

$SAGE_ROOT/devel/sage/sage/matrix/matrix1.pyx

and put it near

    def matrix_from_columns(self, columns):

Or open matrix2.pyx and put it somewhere appropriate. These are Cython files, but hopefully that wouldn't matter here, and perhaps Rob could help review this and make sure it doesn't do anything strange :)

The point is that this is a method of the generic matrix class, NOT a new function, which would be really slow and annoying.

Finally, given the documentation for it,

    def matrix_from_columns(self, columns):
        """
        Return the matrix constructed from self using columns with indices
        in the columns list.

        EXAMPLES::

            sage: M = MatrixSpace(Integers(8),3,3)
            sage: A = M(range(9)); A
            [0 1 2]
            [3 4 5]
            [6 7 0]
            sage: A.matrix_from_columns([2,1])
            [2 1]
            [5 4]
            [0 7]
        """

I would say that you should start from zero as usual in Sage.

[pong]

The patch contains the "delete_columns" and "delete_rows" methods for matrix. I probably had a typo, so the patch has a weird name "...path.patch"

I remove myself from Reviewers. Not sure should I put myself as Authors (so I leave it empty).

[rbeezer]

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.
2. 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.
3. 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.
4. 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.
5. You should use the new Python 3.0 syntax for errors (raise IndexError("explanation") if I remember right), and .format() for formatted strings.
6. 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.
7. 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.

[kcrisman]

A few other things:

• There is now an intended see also formatting -see  this patch, for instance.
• The things to be seen should be hyperlinked. So instead of

  The functions "delete_columns()", "matrix_from_rows()"
  

  one would try to do

  The functions :func:`delete_columns` and :func:`matrix_from_rows`
  

And in case Rob's review seems daunting, be encouraged that these are all both important and not a big deal for you to fix - they are good practice, and not horrible errors of doom haunting your patch for eternity.

[kcrisman]

Assuming those functions are also in the same file, of course.

[rbeezer]

Replying to kcrisman:

Assuming those functions are also in the same file, of course.

Good catch, KDC. I think instead of :func: maybe they should be :meth:?

[kcrisman]

Assuming those functions are also in the same file, of course.

Good catch, KDC. I think instead of :func: maybe they should be :meth:?

Yes, you are right.

[pong]

updated version

[pong]

Replying to kcrisman:

This was requested in the 2011 Sage MAA PREP workshop. Names deleted.

: Is there any way to delete specified rows or columns from an existing matrix?
: Hmm, I think that is possible, using a method.
: I'll look.
:  - we can swap rows, scale rows, add mults of rows, create a matrix from specified rows... but not that.
: Would you find this useful?  Is it in Matlab?
: For list there is  a method called pop
: Right, but not for matrices.
: if matrix is a list of list...
: But it's not quite that.
: I'l make a ticket.
: this would be useful
: there is an m.insert_row
: Hah!  But we can't do it backwards   

So let's make this happen.

Apply trac_11528_matrix_delete_rows_and_columns.patch to devel/sage of sage-5.0beta10

[rbeezer]

Dear Pong,

Making good progress. A few small things to work on. Third patch could be a charm. ;-) You are getting an education, anyway. My time is a whole lot freer now, so it should not take me 5 weeks to swoop in and finish this one off. Keep at it - almost there.

Rob

1. Code uses just check= (good) but documentation still has index_check=, both in the INPUT block and in an example.
2. An example in delete_rows is not formatting right, likely missing a double-color before it begins. You should preview your documentation for gross errors before submitting it for review.

   sage -docbuild reference html
   

   will build the HTML version that you can check in a web browser.
3. The :meth: construction should not have a space before the method name. The effect should be a working link to referenced method, which you can test in the HTML documentation.
4. I'd format True just like dcols, it is literal code.
5. Warnings are rarely used in Sage when an error is more appropriate.

   if not (diff_cols == []):
      warnings.warn("{d} contains invalid indices.".format(d=diff_cols))
   

   If the check is on, it should throw an error (for a calling routine to handle), or if check is off, then it should silently delete the extant rows/columns. Go back to rasing an error like it was originally.
6. Apply section goes in the description, like on other tickets. Part of its purpose is to see which patches to apply. Right now it is not clear that the second patch is incremental and not a replacement. This helps the release manger do a difficult job. I'm going to straighten it out once.
7. We can leave sets like it is, though if anyone every Cythonizes this, it'll probably need reworking.

[pong]

Hi Rob,

I'm now looking at this again. Maybe I overlooked, but I don't see why you said I used just "check=" in the code but used "index_check=" in the documentation. As far as I see (from the patch), I use "index_check=" consistently throughout. Are you suggesting I'd just use "check=" instead of "index_check="? I want to get this clear up before further modification. Let's keep this ball rolling again.

Pong

[rbeezer]

Replying to pong:

Hi Pong,

In the interest of keeping the ball rolling, a quick answer.

I think using just check consistently everywhere is best - that may have been what I meant to convey. This would match existing usage and would allow experienced users to just "guess" at the right keyword (and likely be right).

We'll get this done before Sage Edu Days concludes!

Rob

[pong]

Replying to kcrisman:

This was requested in the 2011 Sage MAA PREP workshop. Names deleted.

: Is there any way to delete specified rows or columns from an existing matrix?
: Hmm, I think that is possible, using a method.
: I'll look.
:  - we can swap rows, scale rows, add mults of rows, create a matrix from specified rows... but not that.
: Would you find this useful?  Is it in Matlab?
: For list there is  a method called pop
: Right, but not for matrices.
: if matrix is a list of list...
: But it's not quite that.
: I'l make a ticket.
: this would be useful
: there is an m.insert_row
: Hah!  But we can't do it backwards   

So let's make this happen.

Apply:

1. trac_11528_delete_rows_and_columns.path.patch
2. trac_11528_matrix_delete_rows_and_columns.patch
3. trac_11528_matrix_delete_rows_columns.patch

[kcrisman]

Which patch is to be applied? Just the last one - trac_11528_matrix_delete_rows_columns.patch - or all three? Just be sure to modify the ticket description (click "Modify Ticket" to see a surprise, all the stuff that used to be obviously visible, sigh).

[pong]

@KDC, I was just going to apologize but trac complains that the ticket maybe invalide since it was modified (apparently by you :P) during the time I typed.

Anyway, only the last patch is to apply. I realized that I should have submitted it as an update but I've already made all the changes, so in order to save me from screwing things up even more, I decided to submit a new patch. I apologize. Well, I hope I finally get everything right in the last patch. Thank both you and Rob for the help.

[kcrisman]

Anyway, only the last patch is to apply. I realized that I should have submitted it as an update but I've already made all the changes, so in order to save me from screwing things up even more, I decided to submit a new patch. I apologize. Well, I hope I finally get everything right in the last patch. Thank both you and Rob for the help.

No apology necessary, I just didn't know which one and wanted to make sure it was clear to any reviewer - someone may beat Rob to the punch! I have a feeling this will be a nice contribution.

[rbeezer]

Needs two things for sure.

1. .:: will make a period and a single colon. One or the other, but we don't want both. The space you removed needs to be there.
2. First error message should be tested.

I have made a reviewer patch addressing these and took the liberty of improving an error message and making the SEEALSO into a complete sentence.

I still need to run tests again on a newer version of Sage - on 4.8 at the office right now.

Pong - you can review my "reviewer patch" and indicate if you agree or not. Somebody will have to review it. If tests are successful I will

(a) go to positive review

(b) fold all the patches into one for the convenience of the release manger.

Right now this needs all 4 patches present.

[rbeezer]

This all is working fine on 5.1.beta0. One thought as I actually used this. Would it make more sense to return dcols rather than diffcols in the error message? In other words return the original, erroneous, input. Or word the error message to say that the list is the complete list of invalid indices rather than saying it contains some invalid indices?

[pong]

Hi Rob,

I have just tested your reviewer patch. It works fine and I like it. There is a tiny tiny point that I want to mention. In the error messages, there should be a full-stop after not {l}.

Pong

[rbeezer]

Standalone, consolidated

[pong]

Rob, I checked it. Looks and works great (the consolidated patch). Please update the status of the ticket.

[rbeezer]

(this post from yesterday got lost)

Consolidated patch includes adjustments to the error messages.

Positive review. (Congratulations on your first contribution.)

Rob