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:

Status badges

Attachments (3)

trac-10847-matrix_plot-subdivisions.patch (5.6 KB) - added by Jason Grout 12 years ago.
trac_10847-reviewer.patch (1.2 KB) - added by Karl-Dieter Crisman 12 years ago.
Attach after main patch
trac_10847-4983-fix.patch (808 bytes) - added by John Palmieri 12 years ago.
apply on top of other patches

Download all attachments as: .zip

Change History (22)

Changed 12 years ago by Jason Grout

comment:1 Changed 12 years ago by Jason Grout

Status: newneeds_review

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

comment:2 Changed 12 years ago by Jason Grout

Cc: Ryan added

comment:3 Changed 12 years ago by Karl-Dieter Crisman

Reviewers: Karl-Dieter Crisman
Status: needs_reviewneeds_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 12 years ago by Rob Beezer

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 Karl-Dieter Crisman

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.

Changed 12 years ago by Karl-Dieter Crisman

Attachment: trac_10847-reviewer.patch added

Attach after main patch

comment:6 Changed 12 years ago by Karl-Dieter Crisman

Description: modified (diff)
Status: needs_workneeds_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 Karl-Dieter Crisman

Status: needs_reviewpositive_review

comment:8 Changed 12 years ago by Jason Grout

#4983 probably conflicts with this patch.

comment:9 Changed 12 years ago by Jason Grout

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

comment:10 Changed 12 years ago by Karl-Dieter Crisman

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?

Changed 12 years ago by John Palmieri

Attachment: trac_10847-4983-fix.patch added

apply on top of other patches

comment:11 Changed 12 years ago by John Palmieri

Description: modified (diff)

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

comment:12 Changed 12 years ago by Jason Grout

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

comment:13 Changed 12 years ago by John Palmieri

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

comment:14 in reply to:  12 Changed 12 years ago by Rob Beezer

Replying to jason:

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.

comment:15 Changed 12 years ago by Jason Grout

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

comment:16 Changed 12 years ago by Rob Beezer

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 12 years ago by Karl-Dieter Crisman

Wow, thanks for the quick work, guys!

comment:18 Changed 12 years ago by Karl-Dieter Crisman

Reviewers: Karl-Dieter CrismanKarl-Dieter Crisman, John Palmieri

comment:19 Changed 11 years ago by Jeroen Demeyer

Merged in: sage-4.7.alpha5
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.