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:  sage6.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 )
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
 Branch set to u/jmantysalo/graphs__is_subgraph_documentation_formatting_error
comment:2 Changed 4 years ago by
 Cc ncohen added
 Commit set to 4015c777f06fab584e247ab929a36d4db7b0d963
 Status changed from new to needs_review
comment:3 Changed 4 years ago by
 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 followup: ↓ 5 Changed 4 years ago by
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 sagedevel?
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 ; followup: ↓ 7 Changed 4 years ago by
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 sagedevel?
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 explicitOUTPUT
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
 Commit changed from 4015c777f06fab584e247ab929a36d4db7b0d963 to b9f84d83799c1c37e50599e5426bd8c7632b8fa2
Branch pushed to git repo; I updated commit sha1. New commits:
b9f84d8  Explicit wording for output "False".

comment:7 in reply to: ↑ 5 Changed 4 years ago by
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 sagedevel?
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 theOUTPUT
sections are mandatory.
But developer guide says "This is not optional.": http://doc.sagemath.org/html/en/developer/coding_basics.html#documentationstrings . I think that this could be corrected. See #17693 comment 16.
comment:8 Changed 4 years ago by
 Status changed from needs_work to needs_review
comment:9 followup: ↓ 12 Changed 4 years ago by
 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
 Cc mlapointe nadialafreniere sschanck added
 Status changed from needs_review to needs_work
comment:11 Changed 4 years ago by
 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
 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
 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
 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 followup: ↓ 18 Changed 4 years ago by
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
 Status changed from needs_review to needs_work
comment:17 Changed 4 years ago by
 Commit changed from a9c5092e45c1627e2c4cac643c7dc583f72307c8 to d45a30b9149367f9e6c660f616d42e30e78178a0
Branch pushed to git repo; I updated commit sha1. New commits:
d45a30b  Corrected phrasing in SEEALSO part of is_subgraph().

comment:18 in reply to: ↑ 15 Changed 4 years ago by
 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
 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
 Branch changed from u/jmantysalo/graphs__is_subgraph_documentation_formatting_error to d45a30b9149367f9e6c660f616d42e30e78178a0
 Resolution set to fixed
 Status changed from positive_review to closed
I changed the wording a little at the same time, when I saw the small formatting error. No code changes.
New commits:
Documentation correction. No code changes.