Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adding method tikz to class Graph #38798

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

seblabbe
Copy link
Contributor

@seblabbe seblabbe commented Oct 10, 2024

Currently, we can create a TikzPicture from a graph as follows, but is raises an experimental feature warning:

sage: from sage.misc.latex_standalone import TikzPicture
sage: g = graphs.PetersenGraph()
sage: t = TikzPicture.from_graph(g)
<ipython-input-20-c4b6306d5e76>:1: FutureWarning: This class/method/function is marked as experimental. It, its functionality or its interface might change without a formal deprecation.
See https://github.com/sagemath/sage/issues/20343 for details.
  t = TikzPicture.from_graph(g)
sage: t

It was declared experimental during the review of #20343 because it should rather be a method of the class Graph.

This is what we do in this PR: we add a tikz method to the class Graph.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation
  • and checked the documentation preview.

⌛ Dependencies

None

Screenshots

As a consequence, the following now works in Jupyter. By default, it uses the dot2tex format if dot2tex is available:

image

Copy link

github-actions bot commented Oct 10, 2024

Documentation preview for this PR (built with commit 2a5b4d7; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it and it is a nice addition.

src/sage/graphs/generic_graph.py Show resolved Hide resolved
src/sage/graphs/generic_graph.py Show resolved Hide resolved
src/sage/graphs/generic_graph.py Show resolved Hide resolved
src/sage/graphs/generic_graph.py Show resolved Hide resolved
src/sage/graphs/generic_graph.py Show resolved Hide resolved
@seblabbe seblabbe requested a review from xcaruso October 17, 2024 09:46
Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much better and the examples are very nice (on my laptop).

- ``rankdir`` -- string (default: ``'down'``), direction of graph layout
when prog is ``'dot'``, possible values are ``'down'``,
``'up'``, ``'right'`` and ``'left'``.
- ``subgraph_clusters`` -- (default: ``[]``) a list of lists of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default is None, not []. The same for other parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None gets replaced by []. This is why I write that the default is []. I don't set the default argument to the empty list due to the gotcha related to doing this (https://docs.python-guide.org/writing/gotchas/).

If you insist that I replace [] by None and that I add a sentence after saying that None gets replaced by [], then I can.

src/sage/graphs/generic_graph.py Show resolved Hide resolved
@fchapoton
Copy link
Contributor

the linter is not happy, please fix the offending line

Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@seblabbe
Copy link
Contributor Author

Si j'ai ajouté Xavier comme reviewer, c'était pas pour mettre de la pression. C'est que j'étais dans son bureau et qu'il était curieux de tester aussi. Dans tous les cas, merci pour la relecture!

@dcoudert
Copy link
Contributor

Pas de souci, et il ne faut pas qu'il hésite à revenir en arrière si il trouve qu'un truc ne va pas.

@xcaruso
Copy link
Contributor

xcaruso commented Oct 18, 2024

Non, tout va bien, c'est parfait.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants