Opened 12 years ago
Closed 11 years ago
#10847 closed enhancement (fixed)
matrix_plot can now plot subdivisions
Reported by: | Jason Grout | Owned by: | jason, was |
---|---|---|---|
Priority: | major | Milestone: | sage-4.7 |
Component: | graphics | Keywords: | |
Cc: | Rob Beezer, Karl-Dieter Crisman, Ryan | Merged in: | sage-4.7.alpha5 |
Authors: | Jason Grout | Reviewers: | Karl-Dieter Crisman, John Palmieri |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
This patch makes matrix_plot learn about plotting subdivisions.
Apply
Attachments (3)
Change History (22)
Changed 12 years ago by
Attachment: | trac-10847-matrix_plot-subdivisions.patch added |
---|
comment:1 Changed 12 years ago by
Status: | new → needs_review |
---|
comment:2 Changed 12 years ago by
Cc: | Ryan added |
---|
comment:3 follow-up: 4 Changed 12 years ago by
Reviewers: | → Karl-Dieter Crisman |
---|---|
Status: | needs_review → needs_work |
This is such a great patch, really nicely put together and documented, everything. I really want to say positive review, except plotting sparse matrices now does not work.
sage: sparse = matrix(dict([((randint(0, 10), randint(0, 10)), 1) for i in xrange(100)])); matrix_plot(sparse) <snip> AttributeError: Unknown property subdivisions
I have no idea how to fix this - all the obvious things I tried, including adding to the list of options deleted for sparse plotting, or deleting this options after creating the lines, did not work. I must be missing something pretty obvious, I guess, but since this is one of the doctest examples it now fails the tests and obviously this needs work!
comment:4 Changed 12 years ago by
Replying to kcrisman:
AttributeError?: Unknown property subdivisions
Matrices have a function subdivision() and an attribute subdivisions.
sage: A= matrix(2, range(4)) sage: type(A.subdivision) <type 'builtin_function_or_method'> sage: type(A.subdivisions) <type 'NoneType'>
The latter should go away, see #4983. Maybe it is not visible outside of a *.pyx file?
Anyway, hope this helps some.
comment:5 Changed 12 years ago by
Well, for some reason doing the right thing (adding to the deleted options for sparse matrices) worked this morning. Patch coming up, once I run relevant tests.
comment:6 Changed 12 years ago by
Description: | modified (diff) |
---|---|
Status: | needs_work → needs_review |
Okay, now all is well, unless someone is *really* tricky and circumvents the decorators and explicitly removes the subdivision keyword. That would also be true for circumventing the vmin keyword, so I judge it to be a non-issue. Positive review.
Apply attachment:trac-10847-matrix_plot-subdivisions.patch and attachment:trac_10847-reviewer.patch
comment:7 Changed 12 years ago by
Status: | needs_review → positive_review |
---|
comment:9 Changed 12 years ago by
(where "conflicts" means that #10847 probably needs to be changed after this patch too.)
comment:10 Changed 12 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 that patch not be based on this instead?
comment:11 Changed 12 years ago by
Description: | modified (diff) |
---|
comment:12 follow-up: 14 Changed 12 years ago by
comment:13 Changed 12 years ago by
#4983 keeps get_subdivisions as a synonym for "subdivisions", for backwards compatibility.
comment:14 Changed 12 years ago by
comment:16 Changed 12 years ago by
I applied #4983, then all the patches here. Passed long tests and patch looks good. So I'll check off on John P's patch as an interested observer, and leave this at positive review.
comment:18 Changed 12 years ago by
Reviewers: | Karl-Dieter Crisman → Karl-Dieter Crisman, John Palmieri |
---|
comment:19 Changed 11 years ago by
Merged in: | → sage-4.7.alpha5 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
I think this is ready for review. I was trying to create a graph like in the Wikipedia page on DCT (http://en.wikipedia.org/wiki/File:Dctjpeg.png), but we didn't support plotting matrix subdivisions and adding lines to a matrix plot was messing it up.
Well, it all works beautifully now, after this patch! :).