Opened 12 years ago
Closed 16 months ago
#5110 closed enhancement (duplicate)
rewrite diagonal_matrix to be more general
Reported by: | jason | Owned by: | was |
---|---|---|---|
Priority: | major | Milestone: | sage-duplicate/invalid/wontfix |
Component: | linear algebra | Keywords: | |
Cc: | Merged in: | ||
Authors: | Reviewers: | Samuel Lelièvre, Travis Scrimshaw | |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
See the patches at #3704, which rewrite diagonal_matrix. On that ticket, however, we finally just went with a basic patch that fixed the specific bug mentioned. There is good code in the other patches that ought to be used, though. This ticket is here so that we eventually use the cleaner rewrite contained in the first patches at #3704.
Change History (10)
comment:1 Changed 8 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:2 Changed 7 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:3 Changed 7 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:4 Changed 7 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:5 Changed 3 years ago by
- Report Upstream set to N/A
comment:6 Changed 3 years ago by
I think the idea is to use attachment:trac-3704-diagonal_matrix.patch:ticket:3704 to make a more generally applicable function.
That said, there are probably more important tickets to work on, notably reviewing many in symbolics ...
comment:7 Changed 3 years ago by
I did see that patch, but looking at the source code for diagonal_matrix(), the differences seem to be
- Changing the parameters
- More comments and doctests
- Casting to Sequence
1 could be a good fix since the current function uses "arg0, arg1, arg2" which is pretty bad, but in my opinion, 2 and 3 are not necessary (although if we do change 1 we will need to change 2).
comment:8 Changed 16 months ago by
- Milestone changed from sage-6.4 to sage-duplicate/invalid/wontfix
- Reviewers set to Samuel Lelièvre
- Status changed from new to needs_review
This is probably solved by #10604 (please check).
comment:9 Changed 16 months ago by
- Status changed from needs_review to positive_review
I think so as well.
comment:10 Changed 16 months ago by
- Resolution set to duplicate
- Reviewers changed from Samuel Lelièvre to Samuel Lelièvre, Travis Scrimshaw
- Status changed from positive_review to closed
What exactly should be added to diagonal_matrix? I'd be happy to work on this ticket but it looks like the other patches in #3704 add doctests.