Opened 9 years ago

Closed 8 years ago

#9211 closed defect (fixed)

graph vertices cut off

Reported by: jason Owned by: jason, mvngu, ncohen, rlm
Priority: major Milestone: sage-4.8
Component: graph theory Keywords:
Cc: rbeezer, kcrisman, kini Merged in: sage-4.8.alpha1
Authors: Jason Grout Reviewers: Punarbasu Purkayastha
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by ppurka)

Though #7299 helped, graph vertices are still cut off.

We can turn off clipping of the matplotlib scatterplot and add some bbox_extra_artists to the savefig which takes those artists into account when calculating the bounding box (when bbox_inches='tight').


First apply trac-9211-fix_cut_vertices_in_graphs.patch and then apply trac_9211_digraph_clipping.patch to $SAGE_ROOT/devel/sage. Finally apply trac_9211-fix_doctests.patch to $SAGE_ROOT/devel/sage.

Attachments (7)

trac-9211-graph-vertices-cut.patch (5.5 KB) - added by jason 9 years ago.
trac-9211-fix_cut_vertices_in_graphs.patch (5.6 KB) - added by ppurka 8 years ago.
( Fix cut off vertices in graphs ) Apply to devel/sage/
trac-9211-add_padding_to_graphs.patch (1.9 KB) - added by ppurka 8 years ago.
( Add additional padding to graphs ) Apply to devel/sage
with-earlier-patch-modified-for-4.7.2.png (24.6 KB) - added by ppurka 8 years ago.
Output with only the earlier patch (modified for 4.7.2)
with-additional-padding.png (21.5 KB) - added by ppurka 8 years ago.
Output with patch for additional padding
trac_9211_digraph_clipping.patch (4.2 KB) - added by jason 8 years ago.
apply after trac-9211-fix_cut_vertices_in_graphs.patch
trac_9211-fix_doctests.patch (3.2 KB) - added by ppurka 8 years ago.
Fix doctests for 3d plots (re-uploaded)

Download all attachments as: .zip

Change History (39)

comment:1 Changed 9 years ago by jason

Here is a good example showing the problem (still):

a=matrix([[1,2],[3,4]])

var('x y z w')
b=matrix([[x,y],[z,w]])

a.set_immutable()
b.set_immutable()

K=DiGraph({a:[b]})
show(K, vertex_size=800)

Changed 9 years ago by jason

comment:2 Changed 9 years ago by jason

  • Status changed from new to needs_review

This patch depends on the matplotlib spkg at #9221. I don't know if the patch is very elegant. I'll trust the reviewer's comments on that. The problem is that it lets the scatter plot vertices extend beyond the frame (relying on the user adjusting the axes_pad option to extend the frame).

comment:3 Changed 9 years ago by jason

  • Authors set to Jason Grout
  • Work issues set to depends on #9221

comment:4 Changed 9 years ago by jason

  • Status changed from needs_review to needs_work

The patch doesn't work for multiple-edge graphs (they are drawn completely differently than graphs without multiple edges). In fact, we probably ought to draw all graphs using CircleCollection? objects rather than scatter_plot or Circles.

comment:5 Changed 9 years ago by jason

Here's how to draw circles like the ones in scatter plot (where the size doesn't change as you make the figure smaller or bigger: use a CircleCollection? argument and use different transforms to specify location and size coordinates. See http://sourceforge.net/mailarchive/forum.php?thread_name=AANLkTim6ZnGiOV1RTRBz0uBMF5y67xyrDp%2BkVSCBtkrB%40mail.gmail.com&forum_name=matplotlib-users

comment:6 Changed 9 years ago by jason

See also http://groups.google.com/group/networkx-discuss/browse_thread/thread/b95fda9813fae67d for code/ideas for rewriting the graph drawing.

comment:7 Changed 9 years ago by jason

I think this patch might take just a little to clean up. Most of the patch is just adding comments to explain the code or reformatting long lines. The big key is the clip=False arguments and the code to add things to the bbox_extra_artists.

I think the patch might just need some doctests and testing. The other comments I put in above can be moved to another ticket that does a bigger revamp of graph drawing.

comment:8 Changed 9 years ago by kcrisman

  • Cc kcrisman added

comment:9 Changed 9 years ago by kcrisman

  • Work issues depends on #9221 deleted

comment:10 Changed 8 years ago by kini

  • Cc kini added

Changed 8 years ago by ppurka

( Fix cut off vertices in graphs ) Apply to devel/sage/

Changed 8 years ago by ppurka

( Add additional padding to graphs ) Apply to devel/sage

Changed 8 years ago by ppurka

Output with only the earlier patch (modified for 4.7.2)

Changed 8 years ago by ppurka

Output with patch for additional padding

comment:11 Changed 8 years ago by ppurka

  • Status changed from needs_work to needs_review

Added two patches. The first one is essentially the patch by jason redone to work with 4.7.2_alpha3.

The second patch adds extra padding to all graphs. The two pngs attached are the output of

M = matrix(RDF, [[1/3, 1/3, 1/3], [0, 1/4, 3/4], [1/2, 1/4, 1/4]])
G=DiGraph(M, format='weighted_adjacency_matrix')
G.show(vertex_size=800, edge_labels=True)

The file with-additional-padding.png is the result of both the patches being applied.

Graphs currently look really bad without either of the two patches. The second patch is not a panacea for all cut off graph plots since it doesn't fix all cases. But my hope is that it fixes most commonly used cases.

comment:12 follow-up: Changed 8 years ago by jason

I'm curious why you need the extra padding patch. Are vertices still cut off using just the first patch?

comment:13 in reply to: ↑ 12 Changed 8 years ago by ppurka

Replying to jason:

I'm curious why you need the extra padding patch. Are vertices still cut off using just the first patch?

Yes. Check the plot after the first patch against the the plot after first+second patch

comment:14 Changed 8 years ago by jason

Ah, right, because the first patch only fixes undirected graphs. You are plotting a directed graph, so the changes in the patch don't apply. I'm uploading a new patch that doesn't clip digraph circles or text.

Changed 8 years ago by jason

apply after trac-9211-fix_cut_vertices_in_graphs.patch

comment:15 follow-up: Changed 8 years ago by jason

Can you try applying just trac-9211-fix_cut_vertices_in_graphs.patch and trac_9211_digraph_clipping.patch ?

comment:16 in reply to: ↑ 15 Changed 8 years ago by ppurka

Replying to jason:

Can you try applying just trac-9211-fix_cut_vertices_in_graphs.patch and trac_9211_digraph_clipping.patch ?

Excellent! Thanks. This does fix the problem for digraphs. Much better than adding random paddings.

comment:17 Changed 8 years ago by ppurka

  • Description modified (diff)

comment:18 Changed 8 years ago by jason

  • Description modified (diff)

comment:19 follow-up: Changed 8 years ago by jason

Okay, did you run long tests on Sage? The new version of my first patch seems okay to me, so if you positively review the patches, I think we are good to go, assuming that you've run long tests and there are no doctest errors.

comment:20 in reply to: ↑ 19 Changed 8 years ago by ppurka

Replying to jason:

Okay, did you run long tests on Sage? The new version of my first patch seems okay to me, so if you positively review the patches, I think we are good to go, assuming that you've run long tests and there are no doctest errors.

I started the long test (make ptestlong) over an hour ago. I guess there's 3 more hours to go :)

comment:21 follow-up: Changed 8 years ago by ppurka

A couple of doctests failed. Only in the directory plot/ (and in the files changed by this patch + base.pyx). Will investigate more once I am physically near that machine where the doctest was run.

comment:22 in reply to: ↑ 21 Changed 8 years ago by kcrisman

A couple of doctests failed. Only in the directory plot/ (and in the files changed by this patch + base.pyx). Will investigate more once I am physically near that machine where the doctest was run.

Just FYI, base.pyx often fails on Mac if you don't have libjpeg or libtiff in the right place - see #7344 and #7345. If the errors look like

    ImportError: The _imaging C module is not installed

you can ignore them, they would be unrelated.

comment:23 Changed 8 years ago by ppurka

  • Description modified (diff)

I am running it on x86_64 Linux. The errors were because of the additional 'clip' option introduced in the patches. Attaching a fix for the doctests.

comment:24 follow-up: Changed 8 years ago by kini

I wonder why you are concatenating lines in the doctest output - our docstrings are theoretically supposed to be 72 (or 79) characters wide, if I'm not mistaken, which is why they the current test results are broken into multiple lines. The doctester doesn't care about purely whitespace differences between the expected and received output.

comment:25 in reply to: ↑ 24 Changed 8 years ago by ppurka

Replying to kini:

I wonder why you are concatenating lines in the doctest output - our docstrings are theoretically supposed to be 72 (or 79) characters wide, if I'm not mistaken, which is why they the current test results are broken into multiple lines. The doctester doesn't care about purely whitespace differences between the expected and received output.

Simply copied the output from "Expected output". Will attached a space-ified fix. :)

comment:26 Changed 8 years ago by kini

I wonder if you are aware of sage -fixdoctests, which does basically exactly what you are doing... :) BTW I have the same comment for trac-9211-add_padding_to_graphs.patch.

Changed 8 years ago by ppurka

Fix doctests for 3d plots (re-uploaded)

comment:27 Changed 8 years ago by ppurka

Uploaded new patch.

Didn't know about -fixdoctests (it wasn't too painful to fix because of vim)

Forget about the adding padding patch. Jason did the real fix. :)

comment:28 Changed 8 years ago by ppurka

  • Reviewers set to Punarbasu Purkayastha
  • Status changed from needs_review to positive_review

Previous failures were:

----------------------------------------------------------------------

The following tests failed:

	sage -t  -long -force_lib devel/sage/sage/plot/scatter_plot.py # 1 doctests failed
	sage -t  -long -force_lib devel/sage/sage/plot/text.py # 5 doctests failed
	sage -t  -long -force_lib devel/sage/sage/plot/circle.py # 8 doctests failed
	sage -t  -long -force_lib devel/sage/sage/plot/plot3d/base.pyx # 5 doctests failed
----------------------------------------------------------------------
Total time for all tests: 8844.0 seconds

New doctest on devel/sage/sage/plot/ passes all doctests:

~/Installations/sage-4.7.2> ./sage -t -long devel/sage/sage/plot
...
----------------------------------------------------------------------
All tests passed!
Total time for all tests: 520.6 seconds

So positive review from my side.

comment:29 Changed 8 years ago by jason

  • Status changed from positive_review to needs_work

comment:30 Changed 8 years ago by jason

  • Status changed from needs_work to needs_review

Your changes were a little more than just fixing whitespace, so let someone look at it to review your last patch.

Sorry---you're right about those doctests. I shouldn't have been so sloppy as to not fix those doctests before indicating it needed to be reviewed.

comment:31 Changed 8 years ago by jason

  • Status changed from needs_review to positive_review

Yep, I confirm that normal tests in graphs/ and plot/ pass. Positive review for the fix doctests patch. Good job!

comment:32 Changed 8 years ago by jdemeyer

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