Opened 10 years ago
Closed 8 years ago
#4983 closed enhancement (fixed)
replace subdivisions attribute for matrices with a function
Reported by: | was | Owned by: | was |
---|---|---|---|
Priority: | minor | Milestone: | sage-4.7 |
Component: | linear algebra | Keywords: | |
Cc: | Merged in: | sage-4.7.alpha4 | |
Authors: | John Palmieri | Reviewers: | Rob Beezer |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
I do not like this:
sage: sage: a = matrix(ZZ,4,[1, 0, 0, 0, 0, 1, 0, 0, 1, -1, 1, 0, 1, -1, 1, 2]) sage: sage: b=a.jordan_form() sage: b.subdivisions ([0, 1, 3, 4], [0, 1, 3, 4]) sage: b.subdivisions = 10 sage: b.subdivisions 10
Notice that you can make the subdivisions nonsense because it can be changed. Also, of course,
sage: b.subdivisions? ... The Integer class represents arbitrary precision integers. It derives from the Element class, so [other useless stuff]
I don't like that at all either. I wish that subdivisions were a method with a proper docstring, doctests, etc., and that variable were hidden.
Then one would do:
sage: b.subdivisions? useful stuff (and also it would be in the reference manual) and sage: b.subdivisions() ([0, 1, 3, 4], [0, 1, 3, 4])
Depends on:
Apply:
Attachments (2)
Change History (21)
comment:1 Changed 10 years ago by
comment:2 Changed 10 years ago by
- Type changed from defect to enhancement
comment:3 Changed 8 years ago by
- Priority changed from major to minor
- Report Upstream set to N/A
- Status changed from new to needs_review
Here's patch. This keeps get_subdivisions
as a synonym for subdivisions
.
comment:4 Changed 8 years ago by
- Reviewers set to Rob Beezer
- Status changed from needs_review to positive_review
This looks real good. Passes long tests. I'm glad to see a "get_" go away.
This means I should mildly change #10974, so I'll go make the changes necessary there and have it depend on this.
comment:5 Changed 8 years ago by
This probably conflicts with #10847 too.
comment:6 Changed 8 years ago by
(where "conflicts" means that #10847 probably needs to be changed after this patch too.)
comment:7 Changed 8 years ago by
It would be nice if a patch that has had positive review for over a week did not have to be rebased for a patch that has had positive review for seven hours. Could this patch not be based on that instead?
comment:8 follow-up: ↓ 9 Changed 8 years ago by
comment:9 in reply to: ↑ 8 Changed 8 years ago by
Replying to jhpalmieri:
kcrisman: I just posted a fix for #10847. (The issue was, the patches at #10847 used the attribute
matrix.subdivisions
instead of using the methodmatrix.get_subdivisions()
.)
Thanks, I appreciate it. I was aware of the incompatibility, just didn't have time to take care of it myself the next few days.
Changed 8 years ago by
comment:10 Changed 8 years ago by
I just uploaded a new patch; the only difference is I added the comment
# 'get_subdivisions' is kept for backwards compatibility: see #4983.
right before get_subdivisions = subdivisions
.
comment:11 Changed 8 years ago by
- Status changed from positive_review to needs_work
This patch conflicts with #10974.
comment:12 follow-up: ↓ 15 Changed 8 years ago by
- Status changed from needs_work to needs_review
Here's a patch rebased against #10974. Does this need review or not?
Changed 8 years ago by
comment:13 Changed 8 years ago by
- Description modified (diff)
comment:14 follow-up: ↓ 17 Changed 8 years ago by
If it's literally a fairly trivial rebase and nothing changed in terms of testing, I think it would be okay to just post a diff of what had to be rebased (since the patch is fairly large) and set back to positive review. If there are some actual nontrivial changes in the code then I guess someone would have to review it.
comment:15 in reply to: ↑ 12 Changed 8 years ago by
Replying to jhpalmieri:
Here's a patch rebased against #10974.
Thanks, John.
Does this need review or not?
Normally, I'd say "not." But I have two or three other rebase tasks for later this afternoon, so I can give it a quick test then.
Rob
comment:16 follow-up: ↓ 18 Changed 8 years ago by
I just rebased it, I didn't change anything else, but if Rob has time to run tests on it, that would be great. (I've already done this, but it's good to double-check it.)
comment:17 in reply to: ↑ 14 Changed 8 years ago by
- Status changed from needs_review to positive_review
comment:18 in reply to: ↑ 16 Changed 8 years ago by
- Description modified (diff)
Replying to jhpalmieri:
(I've already done this, but it's good to double-check it.)
Double-check shows everything is fine on 4.7.alpha3: applies, builds, passes long tests.
Thanks again, John, for sparing me the work on #10974. As a bonus I upgraded the depends/apply block to Jeroen's new formatting. ;-)
comment:19 Changed 8 years ago by
- Merged in set to sage-4.7.alpha4
- Resolution set to fixed
- Status changed from positive_review to closed
The method is
but this should probably be changed to have an attribute _subdivisions and a method subdivisions() for consistency.