Explicitly say that arguments to Graph.plot() are forwarded

[nathanncohen]

As reported in [1], Graph.plot() is not sufficiently explicit about the arguments it accepts.

Nathann

[1] https://groups.google.com/d/topic/sage-devel/XS0EDLqfGtI/discussion

CC: @seblabbe @jm58660

Component: graph theory

Author: Nathann Cohen

Branch/Commit: f3a61f7

Reviewer: Jori Mäntysalo

Issue created by migration from https://trac.sagemath.org/ticket/18646

[nathanncohen]

Branch: public/18646

[nathanncohen]

Commit: 228ff6b

[nathanncohen]

New commits:

228ff6b

trac #18646: Explicitly say that arguments to Graph.plot() are forwarded

[jm58660]

comment:2

LGTM.

[jm58660]

Reviewer: Jori Mäntysalo

[nathanncohen]

comment:3

Thanks !

[seblabbe]

comment:4

I wanted to review this ticket this morning, but it is already positive review. I had one english comment. I don't understand the part "to which they are forwarded." in

            - This method supports any parameter accepted by
              :meth:`sage.plot.graphics.Graphics.show`, to which they are
              forwarded.

Because arguments are forwarded from show to plot not the converse. I would replace "to which" by "from which" or better erase completely "to which they are forwarded." because the user don't care much about what is forwarded where.

I let you guys decide if it is too late or not (maybe this ticket is already being merged).

[nathanncohen]

comment:5

Replying to @seblabbe:

I wanted to review this ticket this morning, but it is already positive review. I had one english comment. I don't understand the part "to which they are forwarded." in

            - This method supports any parameter accepted by
              :meth:`sage.plot.graphics.Graphics.show`, to which they are
              forwarded.

Because arguments are forwarded from show to plot not the converse.

Nonono. The function is Graphics.show, not Graph.show. And in particular Graph.plot forwards its arguments to Graphics.show. This is why, despite having no specific code in Graph.plot to handle "axis", that argument is accepted. Because Graphics.show knows what to make of it.

I let you guys decide if it is too late or not (maybe this ticket is already being merged).

I can remove the sentence if you prefer. In the worst case, the last commit will be ignored by Volker's script.

Nathann

[sagetrac-git]

Changed commit from 228ff6b to f3a61f7

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

f3a61f7

trac #18646: Behead a sentence.

[seblabbe]

comment:8

Replying to @nathanncohen:

Nonono. The function is Graphics.show, not Graph.show. And in particular Graph.plot forwards its arguments to Graphics.show. This is why, despite having no specific code in Graph.plot to handle "axis", that argument is accepted. Because Graphics.show knows what to make of it.

Ok, I see what you meant. But, I still think the last commit it a good idea to avoid confusion.

So if I understand correctly, that "forward" is done indirectly in the ugly line 910 in the method def plot(self, **kwds): of file graph_plot.py:

        G._set_extra_kwds(Graphics._extract_kwds_for_show(options))

[nathanncohen]

comment:9

So if I understand correctly, that "forward" is done indirectly in the ugly line 910 in the method def plot(self, **kwds): of file graph_plot.py:

Yesyes, it seems so.

Nathann

[vbraun]

Changed branch from public/18646 to f3a61f7