matrix_plot can now plot subdivisions

[jasongrout]

This patch makes matrix_plot learn about plotting subdivisions.

---

Apply

• attachment: trac-10847-matrix_plot-subdivisions.patch
• attachment: trac_10847-reviewer.patch
• attachment: trac_10847-4983-fix.patch

CC: @rbeezer @kcrisman @sagetrac-ryan

Component: graphics

Author: Jason Grout

Reviewer: Karl-Dieter Crisman, John Palmieri

Merged: sage-4.7.alpha5

Issue created by migration from https://trac.sagemath.org/ticket/10847

[jasongrout]

Attachment: trac-10847-matrix_plot-subdivisions.patch.gz

[jasongrout]

comment:1

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! :).

[kcrisman]

comment:3

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!

[kcrisman]

Reviewer: Karl-Dieter Crisman

[rbeezer]

comment:4

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.

[kcrisman]

comment:5

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.

[kcrisman]

Attach after main patch

[kcrisman]

comment:6

Attachment: trac_10847-reviewer.patch.gz

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

[jasongrout]

comment:8

#4983 probably conflicts with this patch.

[jasongrout]

comment:9

(where "conflicts" means that #10847 probably needs to be changed after this patch too.)

[kcrisman]

comment:10

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?

[jhpalmieri]

apply on top of other patches

[jhpalmieri]

comment:11

Attachment: trac_10847-4983-fix.patch.gz

I think this patch should fix any incompatibilities with #4983. With this, you can apply the patches here independently of #4983.

[jasongrout]

comment:12

But I thought #4983 got rid of the get_subdivisions() command. Won't you have a problem when you apply #4983 now?

[jhpalmieri]

comment:13

#4983 keeps get_subdivisions as a synonym for "subdivisions", for backwards compatibility.

[rbeezer]

comment:14

Replying to @jasongrout:

But I thought #4983 got rid of the get_subdivisions() command. Won't you have a problem when you apply #4983 now?

#4983, as of about mid-day yesterday, said

Here's patch. This keeps get_subdivisions as a synonym for subdivisions. 

So it was never slated for removal.

[jasongrout]

comment:15

Thanks. I didn't read that patch carefully enough, apparently.

[rbeezer]

comment:16

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.

[kcrisman]

comment:17

Wow, thanks for the quick work, guys!

[kcrisman]

Changed reviewer from Karl-Dieter Crisman to Karl-Dieter Crisman, John Palmieri

[jdemeyer]

Merged: sage-4.7.alpha5