Graphs, is_subgraph() documentation polishing

[jm58660]

There is a missing backstick at generic_graph.py in the documentation of is_subgraph(). This patch contains also some more polishing to that function.

CC: @nathanncohen @sagetrac-mlapointe @nadialafreniere @sagetrac-sschanck

Component: documentation

Author: Jori Mäntysalo

Branch/Commit: d45a30b

Reviewer: Vincent Delecroix, Mélodie Lapointe

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

[jm58660]

Branch: u/jmantysalo/graphs__is_subgraph_documentation_formatting_error

[jm58660]

Commit: 4015c77

[jm58660]

comment:2

I changed the wording a little at the same time, when I saw the small formatting error. No code changes.

---

New commits:

4015c77

Documentation correction. No code changes.

[videlec]

Reviewer: Vincent Delecroix

[videlec]

comment:3

Why is that

- Tests whether ...
+ Return ``True`` if ...

Your version is much less precise since you say nothing when the graph is not a subgraph. It is standard in Sage to write Test Property X. Of course you can remove the s (i.e. Tests -> Test) and remove the reference to self.

The rest looks fine.

Vincent

[jm58660]

comment:4

Well, for now we have for example

is_eulerian() 	Return True if the graph has a - - 
is_planar() 	Test whether the graph is - -
is_circular_planar() 	Test whether the graph is - - 
is_regular() 	Return True if this graph is - -

On #18925 and #18941 I have used "return true if" -phrasing. That can of course be converted, but it should be uniform in all parts of the software. Was there some discussion about this in sage-devel?

And btw, should every function document the output type? As an example, has_bottom() in posets is documented as "Return True if the poset has a unique minimal element." in the index of functions, and then "Return True if the poset has a unique minimal element, and False otherwise." in the function itself. There is no explicit OUTPUT-part.

[videlec]

comment:5

Replying to @jm58660:

Well, for now we have for example

is_eulerian() 	Return True if the graph has a - - 
is_planar() 	Test whether the graph is - -
is_circular_planar() 	Test whether the graph is - - 
is_regular() 	Return True if this graph is - -

On #18925 and #18941 I have used "return true if" -phrasing. That can of course be converted, but it should be uniform in all parts of the software. Was there some discussion about this in sage-devel?

I don't think so. And I agree with you that it would better be uniform.

And btw, should every function document the output type? As an example, has_bottom() in posets is documented as "Return True if the poset has a unique minimal element." in the index of functions, and then "Return True if the poset has a unique minimal element, and False otherwise." in the function itself. There is no explicit OUTPUT-part.

No. Neither the INPUT nor the OUTPUT sections are mandatory. If your function has simple input/output (like a is_X(self) method) then it is fine to include the specifications in the documentation. Otherwise it is up to the programmer to judge whether it is clearer with or without INPUT/OUTPUT.

Vincent

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

b9f84d8

Explicit wording for output "False".

[sagetrac-git]

Changed commit from 4015c77 to b9f84d8

[jm58660]

comment:7

Replying to @videlec:

Replying to @jm58660:

On #18925 and #18941 I have used "return true if" -phrasing. That can of course be converted, but it should be uniform in all parts of the software. Was there some discussion about this in sage-devel?

I don't think so. And I agree with you that it would better be uniform.

Now there is at least explicit wording. I think that it is a place for another ticket to unify the documentation.

And btw, should every function document the output type?

No. Neither the INPUT nor the OUTPUT sections are mandatory.

But developer guide says "This is not optional.": http://doc.sagemath.org/html/en/developer/coding_basics.html#documentation-strings . I think that this could be corrected. See #17693 comment 16.

[sagetrac-mlapointe]

comment:9

-          whether ``self`` is an *induced* subgraph of ``other`` that is if
-          the vertices of ``self`` are also vertices of ``other``, and the
-          edges of  ``self`` are equal to the edges of ``other`` between the
-          vertices contained in ``self`.
+          whether the graph is an *induced* subgraph of ``other`` that is if
+          the vertices of the graph are also vertices of ``other``, and the
+          edges of the poset are equal to the edges of ``other`` between the
+          vertices contained in the graph.

Why self has been changed to poset? Since the function is about graphs and not about posets, the wording should be changed.

[sagetrac-git]

Changed commit from b9f84d8 to a9c5092

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

a9c5092

'poset' -> 'graph'.

[jm58660]

comment:12

Replying to @sagetrac-mlapointe:

Why self has been changed to poset? Since the function is about graphs and not about posets, the wording should be changed.

I had a temporary mental disorder. Corrected.

[sagetrac-mlapointe]

Replying to @jm58660:

There is a missing backstick at generic_graph.py.

You should change the description so it suits to what the ticket finally does.

[sagetrac-mlapointe]

comment:15

I know it is not the main purpose of the patch, but since you have changed self for graph in the documentation maybe you should do it in the section "See also". It looks weird when we build the documentation in html.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

d45a30b

Corrected phrasing in SEEALSO part of is_subgraph().

[sagetrac-git]

Changed commit from a9c5092 to d45a30b

[jm58660]

comment:18

Replying to @sagetrac-mlapointe:

I know it is not the main purpose of the patch, but since you have changed self for graph in the documentation maybe you should do it in the section "See also". It looks weird when we build the documentation in html.

True. I was blind.

Corrected. Needs review.

[sagetrac-mlapointe]

Changed reviewer from Vincent Delecroix to Vincent Delecroix, Mélodie Lapointe

[vbraun]

Changed branch from u/jmantysalo/graphs__is_subgraph_documentation_formatting_error to d45a30b