Ticket #1761 (closed enhancement: fixed)

Opened 5 years ago

Last modified 5 years ago

[with patch, positive review] Graphviz output for graphs

Reported by: boothby Owned by: rlm
Priority: minor Milestone: sage-3.0
Component: graph theory Keywords: graph visualization dot graphviz
Cc: ncalexan Work issues:
Report Upstream: Reviewers:
Authors: Merged in:
Dependencies: Stopgaps:

Description

Add functionality to use graphviz for graph display.

Attachments

graphviz.hg Download (2.2 KB) - added by boothby 5 years ago.
1761-ncalexan-graphviz-1.patch Download (5.9 KB) - added by ncalexan 5 years ago.
trac_1761-doctest-fix.patch Download (676 bytes) - added by mabshoff 5 years ago.
doctest fix due to bitrot
trac_1761-tmp-file-dir-fix.patch Download (1.1 KB) - added by mabshoff 5 years ago.
create doctest files in SAGE_TESTDIR

Change History

Changed 5 years ago by boothby

comment:1 Changed 5 years ago by mhansen

  • Summary changed from Graphviz output for graphs to [with bundle, needs review] Graphviz output for graphs

comment:2 Changed 5 years ago by mabshoff

  • Milestone set to sage-2.10

comment:3 Changed 5 years ago by ncalexan

  • Summary changed from [with bundle, needs review] Graphviz output for graphs to [with bundle, with mostly positive review] Graphviz output for graphs

This is useful and should be applied. I would have liked to see the actual function abstracted to a Graph class higher in the hierarchy, if there is one, because it seems like the actual code is the same, just the edge identifier symbols for dot are different. Also, is '# not tested' the correct tag for doctests to not get run?

comment:4 Changed 5 years ago by jason

  • Summary changed from [with bundle, with mostly positive review] Graphviz output for graphs to [with bundle, with two mostly positive reviews, but changes recommended] Graphviz output for graphs

I agree with ncalexan:

  1. The string functions should be abstracted to the GenericGraph? class with the string changing depending on whether the graph is directed or not. In general, we are trying to consolidate functionality to the GenericGraph? class when possible these days.
  1. The graphviz function documentation should clearly state that graphviz (and in particular, dot) needs to be installed in the system path. It would be nice were the option to run the other graphviz programs, like neato, etc.

comment:5 Changed 5 years ago by ncalexan

  • Cc ncalexan added
  • Keywords graph visualization dot graphviz added
  • Summary changed from [with bundle, with two mostly positive reviews, but changes recommended] Graphviz output for graphs to [with patch, needs review] Graphviz output for graphs

The attached patch addresses the referee's comments.

It also removes references to the actual graphviz/dot application. I don't see how this can be made cross-platform and I'm worried about licensing issues, so I'm just ducking the issue. Open a new ticket if you'd like this functionality.

Changed 5 years ago by ncalexan

comment:6 Changed 5 years ago by ncalexan

The original bundle is not needed; apply only the patch.

comment:7 Changed 5 years ago by boothby

  • Summary changed from [with patch, needs review] Graphviz output for graphs to [with patch, positive review] Graphviz output for graphs

Looks good to me.

comment:8 Changed 5 years ago by mabshoff

  • Status changed from new to closed
  • Resolution set to fixed

Merged in Sage 3.0.alpha3

Changed 5 years ago by mabshoff

doctest fix due to bitrot

Changed 5 years ago by mabshoff

create doctest files in SAGE_TESTDIR

Note: See TracTickets for help on using tickets.