Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#10349 closed enhancement (fixed)

Implementation of mutations for matrices

Reported by: stumpc5 Owned by:
Priority: major Milestone: sage-5.0
Component: combinatorics Keywords: matrix mutation
Cc: hthomas@… Merged in: sage-5.0.beta6
Authors: Christian Stump Reviewers: Hugh Thomas
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #10347 Stopgaps:

Description (last modified by stumpc5)

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

Attachments (1)

trac_10349_matrix_mutation-cs.patch (3.2 KB) - added by stumpc5 6 years ago.
contains matrix method mutate

Download all attachments as: .zip

Change History (22)

comment:1 Changed 7 years ago by stumpc5

  • Component changed from PLEASE CHANGE to combinatorics
  • Status changed from new to needs_review

comment:2 Changed 7 years ago by stumpc5

  • Description modified (diff)

comment:3 Changed 7 years ago by stumpc5

For the buildbot:

Depends on #10347.

comment:4 Changed 6 years ago by stumpc5

  • Dependencies set to 10347
  • Description modified (diff)

comment:5 Changed 6 years ago by stumpc5

  • Dependencies changed from 10347 to #10347
  • Owner changed from tbd to (none)

comment:6 Changed 6 years ago by hthomas

  • Cc hthomas@… added
  • Reviewers set to Hugh Thomas
  • Status changed from needs_review to needs_work

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

comment:7 Changed 6 years ago by hthomas

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

cheers

Hugh

comment:8 follow-up: Changed 6 years ago by stumpc5

Dear Hugh,

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

Christian

comment:9 Changed 6 years ago by stumpc5

  • Status changed from needs_work to needs_review

comment:10 Changed 6 years ago by stumpc5

  • Milestone changed from sage-4.7.1 to sage-4.7.2

comment:11 in reply to: ↑ 8 Changed 6 years ago by stumpc5

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

comment:12 Changed 6 years ago by hthomas

I'll take a look.

cheers,

Hugh

comment:13 follow-up: Changed 6 years ago by hthomas

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

comment:14 Changed 6 years ago by hthomas

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.

comment:15 in reply to: ↑ 13 Changed 6 years ago by stumpc5

  • Description modified (diff)

Replying to hthomas:

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.

comment:16 follow-up: Changed 6 years ago by stumpc5

  • Description modified (diff)

comment:17 in reply to: ↑ 16 ; follow-up: Changed 6 years ago by hthomas

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.

Changed 6 years ago by stumpc5

contains matrix method mutate

comment:18 in reply to: ↑ 17 Changed 6 years ago by stumpc5

Replying to hthomas:

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!

comment:19 Changed 6 years ago by hthomas

  • Status changed from needs_review to positive_review

Okay, then, I think it's ready.

cheers,

Hugh

comment:20 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.0.beta6
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:21 Changed 6 years ago by nthiery

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
Note: See TracTickets for help on using tickets.