Fixed docstrings for graph relabeling and sublattice mapping functionality missing from reference documentation. #237
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #237 +/- ##
=======================================
Coverage 75.81% 75.81%
=======================================
Files 31 31
Lines 2171 2171
=======================================
Hits 1646 1646
Misses 525 525 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
A couple of general comments:
- Please make all PRs on a new branch, not on
main - I made some comments on the first docstrings that apply to the rest as well, so I did not repeat them for all: these are about the use of the descriptive mode versus command for the first line, the use of back ticks for parameter references, and the Notes sections.
| @@ -252,7 +252,7 @@ def checkadd(v, q): | |||
|
|
|||
|
|
|||
| def find_chimera_indices(G): | |||
| """Attempts to determine the Chimera indices of the nodes in graph G. | |||
| """Attempt to determine the Chimera indices of the nodes in graph G. | |||
There was a problem hiding this comment.
The dwave_networkx package is the one exception we have to the standard PEP "prescribes the function or method’s effect as a command" to be consistent with NetworkX (although it seems to me that NetworkX is now more compliant with PEP than it was in the past).
For internal consistency we should keep using the descriptive mode for the first line until a time when/if we convert all the docstrings
| @@ -496,7 +496,7 @@ def iter_linear_to_chimera_pairs(self, plist): | |||
| return self._pair_repack(self.iter_linear_to_chimera, plist) | |||
|
|
|||
| def graph_to_linear(self, g): | |||
| """Return a copy of the graph g relabeled to have linear indices""" | |||
| """Return a copy of the graph `g` relabeled to have linear indices.""" | |||
There was a problem hiding this comment.
| """Return a copy of the graph `g` relabeled to have linear indices.""" | |
| """Returns a copy of the graph ``g`` relabeled to have linear indices.""" |
There was a problem hiding this comment.
Should really have an Args section with a description of the parameter
| @@ -515,7 +515,7 @@ def graph_to_linear(self, g): | |||
| ) | |||
|
|
|||
| def graph_to_chimera(self, g): | |||
| """Return a copy of the graph g relabeled to have chimera coordinates""" | |||
| """Return a copy of the graph `g` relabeled to have Chimera coordinates.""" | |||
There was a problem hiding this comment.
Same comments: "Returns" and "g"
| @@ -545,7 +545,7 @@ def __missing__(self, key): | |||
| _chimera_coordinates_cache = __chimera_coordinates_cache_dict() | |||
|
|
|||
| def linear_to_chimera(r, m, n=None, t=None): | |||
| """Convert the linear index `r` into a chimera index. | |||
| """Convert the linear index `r` into a Chimera index. | |||
There was a problem hiding this comment.
Same comments: "Converts" and "r"
| @@ -545,7 +545,7 @@ def __missing__(self, key): | |||
| _chimera_coordinates_cache = __chimera_coordinates_cache_dict() | |||
|
|
|||
| def linear_to_chimera(r, m, n=None, t=None): | |||
There was a problem hiding this comment.
Is this function not documented anywhere?
There was a problem hiding this comment.
Could you elaborate? linear_to_chimera does have a docstring.
There was a problem hiding this comment.
There's a linear_to_chimera method of chimera_coordinates defined on line 406 and a function linear_to_chimera defined on line 547, but I see only the first documented on line 40 of utilities.rst
| set of sublattice mappings would take those isomorphisms into account; we do | ||
| not undertake that complexity here. | ||
|
|
||
| """Yield mappings from a Chimera graph into a larger Chimera graph. |
There was a problem hiding this comment.
Same comment about internal consistency
| target : NetworkX Graph | ||
| The Chimera graph that nodes are input from | ||
| The Chimera graph that nodes are output to. |
| is stored in ``mapping.offset``, which can be collected and passed | ||
| into ``offset_list`` in a later session. | ||
|
|
||
| Notes |
There was a problem hiding this comment.
I like the idea of using a different section to structure long descriptive content but these docstrings are now inconsistent with the rest of the documentation: the majority of docstrings use the Notes section for minor comments; see for example maximum_independent_set
|
Closing as redundant to #238 |
Also, edited those docstrings.
This is the fix for issue #234.