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:

Attachments (3)

trac-10847-matrix_plot-subdivisions.patch (5.6 KB) - added by jason 9 years ago.
trac_10847-reviewer.patch (1.2 KB) - added by kcrisman 9 years ago.
Attach after main patch
trac_10847-4983-fix.patch (808 bytes) - added by jhpalmieri 9 years ago.
apply on top of other patches

Download all attachments as: .zip

Change History (22)

Changed 9 years ago by jason

comment:1 Changed 9 years ago by jason

  • Status changed from new to needs_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 9 years ago by jason

  • Cc ryan added

comment:3 follow-up: Changed 9 years ago by kcrisman

  • 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 rbeezer

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 kcrisman

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 9 years ago by kcrisman

Attach after main patch

comment:6 Changed 9 years ago by kcrisman

  • 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 kcrisman

  • Status changed from needs_review to positive_review

comment:8 Changed 9 years ago by jason

#4983 probably conflicts with this patch.

comment:9 Changed 9 years ago by jason

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

comment:10 Changed 9 years ago by kcrisman

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 9 years ago by jhpalmieri

apply on top of other patches

comment:11 Changed 9 years ago by jhpalmieri

  • 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 follow-up: Changed 9 years ago by jason

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 9 years ago by jhpalmieri

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

comment:14 in reply to: ↑ 12 Changed 9 years ago by rbeezer

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 9 years ago by jason

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

comment:16 Changed 9 years ago by rbeezer

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 kcrisman

Wow, thanks for the quick work, guys!

comment:18 Changed 9 years ago by kcrisman

  • Reviewers changed from Karl-Dieter Crisman to Karl-Dieter Crisman, John Palmieri

comment:19 Changed 9 years ago by jdemeyer

  • Merged in set to sage-4.7.alpha5
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.