Opened 4 years ago

Closed 4 years ago

#19008 closed defect (fixed)

Graphs, is_subgraph() documentation polishing

Reported by: jmantysalo Owned by:
Priority: trivial Milestone: sage-6.9
Component: documentation Keywords:
Cc: ncohen, mlapointe, nadialafreniere, sschanck Merged in:
Authors: Jori Mäntysalo Reviewers: Vincent Delecroix, Mélodie Lapointe
Report Upstream: N/A Work issues:
Branch: d45a30b (Commits) Commit: d45a30b9149367f9e6c660f616d42e30e78178a0
Dependencies: Stopgaps:

Description (last modified by jmantysalo)

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.

Change History (20)

comment:1 Changed 4 years ago by jmantysalo

  • Branch set to u/jmantysalo/graphs__is_subgraph_documentation_formatting_error

comment:2 Changed 4 years ago by jmantysalo

  • Cc ncohen added
  • Commit set to 4015c777f06fab584e247ab929a36d4db7b0d963
  • Status changed from new to needs_review

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


New commits:

4015c77Documentation correction. No code changes.

comment:3 Changed 4 years ago by vdelecroix

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to needs_work

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

comment:4 follow-up: Changed 4 years ago by jmantysalo

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.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 4 years ago by vdelecroix

Replying to jmantysalo:

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

comment:6 Changed 4 years ago by git

  • Commit changed from 4015c777f06fab584e247ab929a36d4db7b0d963 to b9f84d83799c1c37e50599e5426bd8c7632b8fa2

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

b9f84d8Explicit wording for output "False".

comment:7 in reply to: ↑ 5 Changed 4 years ago by jmantysalo

Replying to vdelecroix:

Replying to jmantysalo:

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.

comment:8 Changed 4 years ago by jmantysalo

  • Status changed from needs_work to needs_review

comment:9 follow-up: Changed 4 years ago by mlapointe

-          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.

comment:10 Changed 4 years ago by mlapointe

  • Cc mlapointe nadialafreniere sschanck added
  • Status changed from needs_review to needs_work

comment:11 Changed 4 years ago by git

  • Commit changed from b9f84d83799c1c37e50599e5426bd8c7632b8fa2 to a9c5092e45c1627e2c4cac643c7dc583f72307c8

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

a9c5092'poset' -> 'graph'.

comment:12 in reply to: ↑ 9 Changed 4 years ago by jmantysalo

  • Status changed from needs_work to needs_review

Replying to 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.

comment:13 in reply to: ↑ description Changed 4 years ago by mlapointe

  • Status changed from needs_review to needs_work

Replying to jmantysalo:

There is a missing backstick at generic_graph.py.

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

comment:14 Changed 4 years ago by jmantysalo

  • Description modified (diff)
  • Status changed from needs_work to needs_review
  • Summary changed from Graphs, is_subgraph documentation formatting error to Graphs, is_subgraph() documentation polishing

comment:15 follow-up: Changed 4 years ago by 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.

comment:16 Changed 4 years ago by mlapointe

  • Status changed from needs_review to needs_work

comment:17 Changed 4 years ago by git

  • Commit changed from a9c5092e45c1627e2c4cac643c7dc583f72307c8 to d45a30b9149367f9e6c660f616d42e30e78178a0

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

d45a30bCorrected phrasing in SEEALSO part of is_subgraph().

comment:18 in reply to: ↑ 15 Changed 4 years ago by jmantysalo

  • Status changed from needs_work to needs_review

Replying to 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.

comment:19 Changed 4 years ago by mlapointe

  • Reviewers changed from Vincent Delecroix to Vincent Delecroix, Mélodie Lapointe
  • Status changed from needs_review to positive_review

comment:20 Changed 4 years ago by vbraun

  • Branch changed from u/jmantysalo/graphs__is_subgraph_documentation_formatting_error to d45a30b9149367f9e6c660f616d42e30e78178a0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.