Opened 9 years ago
Closed 9 years ago
#10847 closed enhancement (fixed)
matrix_plot can now plot subdivisions
Reported by: | jason | Owned by: | jason, was |
---|---|---|---|
Priority: | major | Milestone: | sage-4.7 |
Component: | graphics | Keywords: | |
Cc: | rbeezer, kcrisman, 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 9 years ago by
comment:1 Changed 9 years ago by
- Status changed from new to needs_review
comment:2 Changed 9 years ago by
- Cc ryan added
comment:3 follow-up: ↓ 4 Changed 9 years ago by
- Reviewers set to Karl-Dieter Crisman
- Status changed from needs_review to 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 in reply to: ↑ 3 Changed 9 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 9 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 9 years ago by
- Description modified (diff)
- Status changed from needs_work to 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 9 years ago by
- Status changed from needs_review to positive_review
comment:8 Changed 9 years ago by
#4983 probably conflicts with this patch.
comment:9 Changed 9 years ago by
(where "conflicts" means that #10847 probably needs to be changed after this patch too.)
comment:10 Changed 9 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 9 years ago by
- Description modified (diff)
comment:12 follow-up: ↓ 14 Changed 9 years ago by
comment:13 Changed 9 years ago by
#4983 keeps get_subdivisions as a synonym for "subdivisions", for backwards compatibility.
comment:14 in reply to: ↑ 12 Changed 9 years ago by
comment:15 Changed 9 years ago by
Thanks. I didn't read that patch carefully enough, apparently.
comment:16 Changed 9 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:17 Changed 9 years ago by
Wow, thanks for the quick work, guys!
comment:18 Changed 9 years ago by
- Reviewers changed from Karl-Dieter Crisman to Karl-Dieter Crisman, John Palmieri
comment:19 Changed 9 years ago by
- Merged in set to sage-4.7.alpha5
- Resolution set to fixed
- Status changed from positive_review to 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! :).