#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 )
This patch implements a method to mutate matrices. This method is needed for the cluster algebra and quiver package, see Ticket #10298.
Attachments (1)
Change History (22)
comment:1 Changed 7 years ago by
- Component changed from PLEASE CHANGE to combinatorics
- Status changed from new to needs_review
comment:2 Changed 7 years ago by
- Description modified (diff)
comment:3 Changed 7 years ago by
comment:4 Changed 6 years ago by
- Dependencies set to 10347
- Description modified (diff)
comment:5 Changed 6 years ago by
- Dependencies changed from 10347 to #10347
- Owner changed from tbd to (none)
comment:6 Changed 6 years ago by
- 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
Oh, one more comment, in the doc-string, you have "b-matrix"; I'd prefer "B-matrix".
cheers
Hugh
comment:8 follow-up: ↓ 11 Changed 6 years ago by
Dear Hugh,
I added the changes you suggested -- thanks a lot for looking at it!
Christian
comment:9 Changed 6 years ago by
- Status changed from needs_work to needs_review
comment:10 Changed 6 years ago by
- Milestone changed from sage-4.7.1 to sage-4.7.2
comment:11 in reply to: ↑ 8 Changed 6 years ago by
comment:12 Changed 6 years ago by
I'll take a look.
cheers,
Hugh
comment:13 follow-up: ↓ 15 Changed 6 years ago by
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
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
- 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: ↓ 17 Changed 6 years ago by
- Description modified (diff)
comment:17 in reply to: ↑ 16 ; follow-up: ↓ 18 Changed 6 years ago by
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.
comment:18 in reply to: ↑ 17 Changed 6 years ago by
comment:19 Changed 6 years ago by
- Status changed from needs_review to positive_review
Okay, then, I think it's ready.
cheers,
Hugh
comment:20 Changed 6 years ago by
- 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
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
For the buildbot:
Depends on #10347.