Implementation of mutations for matrices

[stumpc5]

This patch implements a method to mutate matrices. This method is needed for the cluster algebra and quiver package, see Ticket #10298.

Depends on #10347

CC: @hughrthomas

Component: combinatorics

Keywords: matrix mutation

Author: Christian Stump

Reviewer: Hugh Thomas

Merged: sage-5.0.beta6

Issue created by migration from https://trac.sagemath.org/ticket/10349

[stumpc5]

comment:3

For the buildbot:

Depends on #10347.

[stumpc5]

Dependencies: 10347

[stumpc5]

Changed dependencies from 10347 to #10347

[hughrthomas]

Reviewer: Hugh Thomas

[hughrthomas]

comment:6

Hi Christian--

What do you think about the idea of checking that 0 <= k < min(ncols,nrows)? Your code is very nicely written to be safe even if someone tries to mutate a 2x2 matrix at row 15, but they shouldn't be doing that. More importantly, they shouldn't be trying to mutate a 2n x n matrix at row j for n<= j < 2n -- for such a parameter, mutation is undefined. Returning what looks like meaningful output in such a case is potentially deceptive. (That's my view, at any rate, though if you disagree, I could be convinced otherwise.)

It's also true that the principal part of B should be skew-symmetrizable, but I don't think it's necessary to check that -- though maybe it would be good to mention this in the documentation? (The legit range of k values should also go in the documentation, I guess.)

And, Zelevinsky :)

Otherwise, though, it looks good.

cheers,

Hugh

[hughrthomas]

comment:7

Oh, one more comment, in the doc-string, you have "b-matrix"; I'd prefer "B-matrix".

cheers

Hugh

[stumpc5]

comment:8

Dear Hugh,

I added the changes you suggested -- thanks a lot for looking at it!

Christian

[stumpc5]

comment:11

Replying to @stumpc5:

I just pushed a new version which should be compatible with the #10347, if you also wanna have a look here...

Best, Christian

[hughrthomas]

comment:12

I'll take a look.

cheers,

Hugh

[hughrthomas]

comment:13

Hi Christian--

What's the point of declaring k to be of type Py_ssize_t? I thought the point was that this would allow k to be huge, but in that case, it seems that you would get overflows when you run into the fact that i and j are declared to be integers.

cheers,

Hugh

[hughrthomas]

comment:14

Otherwise, I am happy. The patchbot was happy with this patch before, right? So I assume that its present unhappiness (?) is unrelated to the patch itself, so I guess all that's left is the question in my previous comment.

[stumpc5]

comment:15

Replying to @hughrthomas:

Hi Christian--

What's the point of declaring k to be of type Py_ssize_t? I thought the point was that this would allow k to be huge, but in that case, it seems that you would get overflows when you run into the fact that i and j are declared to be integers.

Hi Hugh,

Py_ssize_t is the recommended integer type for cython, see http://www.python.org/dev/peps/pep-0353/. In fact, in our current situation there is no advantage of using this type over using int. It is used all over in the code for matrices, see for example the method get_unsafe - that's why I also used it.

[hughrthomas]

comment:17

Replying to @stumpc5:

Then why not declare i,j,_ also to be the same type? It seems to me that if it's ever useful to have chosen that type, you would also have needed i,j,_ to have been of that type.

[stumpc5]

contains matrix method mutate

[stumpc5]

comment:18

Attachment: trac_10349_matrix_mutation-cs.patch.gz

Replying to @hughrthomas:

Replying to @stumpc5:

Then why not declare i,j,_ also to be the same type? It seems to me that if it's ever useful to have chosen that type, you would also have needed i,j,_ to have been of that type.

Done!

[hughrthomas]

comment:19

Okay, then, I think it's ready.

cheers,

Hugh

[jdemeyer]

Merged: sage-5.0.beta6

[nthiery]

comment:21

Hi!

This patch is merged in, and we want the cluster algebra material to get in. So this comment is for the future.

The functionality implemented in this ticket is rather specific to the context of cluster algebras. The average matrix user will have no clue what this is about, and might get confused with the unrelated concept of mutability (as in set_immutable). So I would recommend, in a later ticket, to:

• Rename this method to something more specific like cluster_algebra_mutate
  Maybe even move it into the cluster algebra library
• Add a minimal description of what the method actually does
• Add links to the cluster algebra library / tutorial (when it exists)
• Add link to the _check_symmetrizability