Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix typos and styling. #226

Merged
merged 6 commits into from
Mar 20, 2023
Merged

Conversation

qwriter
Copy link
Contributor

@qwriter qwriter commented Feb 3, 2023

Copyedit for clarity.
Add reference documentation for the following methods because they were not being generated:

  • chimera_node_placer_2d
  • draw_chimera_embedding
  • draw_chimera_yield
  • draw_pegasus_yield
  • zephyr_node_placer_2d

Copyedit for clarity.
Add reference documentation for the following methods because they were not being generated:
   chimera_node_placer_2d
   draw_chimera_embedding
   draw_chimera_yield
   draw_pegasus_yield
   zephyr_node_placer_2d
with the exception of the `pos` parameter which is not used by this
function. If `linear_biases` or `quadratic_biases` are provided,
any provided `node_color` or `edge_color` arguments are ignored.
Parameters in :func:`~networkx.drawing.nx_pylab.draw_networkx`, except for the ``pos`` parameter.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it work with the simpler path networkx.draw_networkx, it will be less fragile to changes in structure than networkx.drawing.nx_pylab.draw_networkx.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. @JoelPasvolsky?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also don't think so, networkx.drawing.nx_pylab.draw_networkx reference/generated/networkx.drawing.nx_pylab.draw_networkx.html#networkx.drawing.nx_pylab.draw_networkx is the only draw_networkx under py:function in the NetworkX objects.inv file that I see

Copy link
Contributor

@JoelPasvolsky JoelPasvolsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@qwriter, I looked at the Chimera updates and made some general comments at are applicable to the other files as well. I can do a 2nd full round later

@@ -28,33 +28,34 @@


def chimera_layout(G, scale=1., center=None, dim=2):
"""Positions the nodes of graph G in a Chimera cross topology.
"""Position the nodes of graph ``G`` in a Chimera cross topology.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arcondello, If I remember correctly, we made dwave-networkx an exception to the Python PEP style of using the imperative to conform with the networkx style instead.


scale : float (default 1.)
Scale factor. When scale = 1, all positions fit within [0, 1]
Scale factor. If ``scale = 1``, then all positions fit within [0, 1]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine, but you can reduce the amount of formatted text with something like "When scale is 1, all positions" (also removes the unneeded "then". It's worth keeping formatted text to a minimum, looks nicer

on the x-axis and [-1, 0] on the y-axis.

center : None or array (default None)
Coordinates of the top left corner.

dim : int (default 2)
Number of dimensions. When dim > 2, all extra dimensions are
Number of dimensions. If ``dim > 2``, then all extra dimensions are
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can drop the "then". (Applied elsewhere too)
An alternative to consider (less formatted text): "For dim greater than 2, all extra ..."

Copy link
Contributor Author

@qwriter qwriter Mar 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the mathematical symbols more succinct and audience appropriate?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to this comment, weigh additional formatting versus succinctness of math instead of text. I find it most helpful to look at the rendered HTML for the section to feel if it's starting to look checkerboardish.

Copy link
Contributor Author

@qwriter qwriter Mar 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the HTML and I think it looks okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also a bit hard in API docs not to have a lot of syntax highlighting; it's just the nature of API docs.


If `linear_biases` and/or `quadratic_biases` are provided, these
are visualized on the plot.
Linear and quadratic biases are visualized on the plot as specified
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Linear and quadratic biases are visualized on the plot as specified
Linear and quadratic biases are visualized on the plot if specified

The new wording of the intro makes it seem mandatory

Comment on lines 204 to 205
A dict of biases associated with each node in ``G`` and of
the form ``{node: bias, ...}``. Each bias is numeric.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
A dict of biases associated with each node in ``G`` and of
the form ``{node: bias, ...}``. Each bias is numeric.
Linear biases for all nodes of ``G``, as a dict of form ``{node: bias, ...}``,
where ``bias`` is numeric.

We prefer to start the description with what the parameter means ("linear biases") rather than the data type ("dict"). The above is more consistent with our docs

Also applies elsewhere below

If interaction_edges is not None, then only display the couplers in that
list. If embedded_graph is not None, the only display the couplers between
chains with intended couplings according to embedded_graph.
If the ``interaction_edges`` parameter is not None, then only display the couplers in that
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this intro paragraph? Why can't this all be specified in the descriptions of the parameters themselves? The intro should be to point out general, crucial, or unintuitive aspects of the function

Graph which contains all keys of the ``emb`` parameter as nodes. If specified, then
the edges of ``G`` will be considered interactions if and only if they
exist between two chains of the ``emb`` parameter and if their keys are connected by
an edge in the ``embedded_graph`` parameter.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it is helpful to use a "the x parameter" for the first usage to call attention that the formatted text is params listed and described here, but three uses adds text with decreasing marginal utility


interaction_edges : list (optional, default None)
A list of edges which will be used as interactions.
List of edges which will be used as interactions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
List of edges which will be used as interactions.
Interactions as a list of edges.

in chains, and edges which are neither chain edges nor interactions.
If unused_color is None, these nodes and edges will not be shown at all.
Color to use for graph ``G``'s nodes that are not part of
chains and edges that are neither chain edges nor interactions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ambiguous phrasing: it's unclear whether it is saying "part of chains and edges that are" or "... and edges that are"

If unused_color is None, these nodes and edges will not be shown at all.
Color to use for graph ``G``'s nodes that are not part of
chains and edges that are neither chain edges nor interactions.
If None, then these nodes and edges will not be shown.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another reminder that you can drop all these "then"s

@@ -28,33 +28,34 @@


def chimera_layout(G, scale=1., center=None, dim=2):
"""Positions the nodes of graph G in a Chimera cross topology.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrt this comment, could we delete the phrase "in a Chimera cross topology"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function creates a Chimera layout that can then be plotted. It could have potentially realized the layout in different ways, in particular, the cross and column layouts shown here. The modifier "in a Chimera cross topology" is saying that the function selected to implement the cross layout.
"topology" is not the best word choice because it's used for the QPU connectivity (i.e. Chimera structure). So the info is needed but needs improved phrasing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"in a Chimera cross layout"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be okay or perhaps something like, "Positions a graph's nodes in a Chimera layout with unit cells as crosses."
Kelly uses "cross configuration" in Pegasus but I'm not sure that's clear to most users.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the HTML rendering, it seems to me that the modifier "with unit cells as crosses" is given too much importance at the top-level by being in the one-line descriptions. For someone looking for Chimera graph functions, what's important at the top level is that function chimera_layout "Positions the nodes of a graph in a Chimera layout."
If we provided column-layout too, it would make sense to specify at the top level the cross/column info. We just have the one option, so I'm inclined to put that in a separate, next line.

image

Copy link
Contributor

@JoelPasvolsky JoelPasvolsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments on the Chimera functions so far. Will get to the Pegasus & Zephyr later

@@ -4,12 +4,12 @@
Drawing
*******

Tools to visualize topologies of D-Wave QPUs and weighted graph problems on them.
Tools to visualize topologies of D-Wave QPUs and weighted :term:`graph` problems on them.

.. currentmodule:: dwave_networkx

.. note:: Some functionality requires `NumPy <https://scipy.org>`_ and/or
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"requires NumPy <https://scipy.org>_ and/or" --> both dwave-networkx's dependencies, dimod and networkx, have NumPy as a dependency. Is there any chance dwave-networkx users won't have NumPy? @arcondello

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. We don't have it as a full dependency so theoretically NetworkX and dimod could drop their dependencies on it. That said, making it a full dependency seems reasonable to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be a future enhancement? Do I need to update this sentence?

@@ -28,33 +28,34 @@


def chimera_layout(G, scale=1., center=None, dim=2):
"""Positions the nodes of graph G in a Chimera cross topology.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the HTML rendering, it seems to me that the modifier "with unit cells as crosses" is given too much importance at the top-level by being in the one-line descriptions. For someone looking for Chimera graph functions, what's important at the top level is that function chimera_layout "Positions the nodes of a graph in a Chimera layout."
If we provided column-layout too, it would make sense to specify at the top level the cross/column info. We just have the one option, so I'm inclined to put that in a separate, next line.

image


Graph H (blue) overlaid on a Chimera unit cell (red nodes and black edges).
Graph H (blue) overlaid on a Chimera unit cell (red nodes and black edges).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing :code:`` (or backtic) for the H.
Here's a good place to unobtrusively point out that this Chimera unit cell is rendered in cross layout (because we refer to that in the functions but don't show cross versus column to really clarify that [which we don't do because column drawing is not supported])

@@ -28,33 +28,34 @@


def chimera_layout(G, scale=1., center=None, dim=2):
"""Positions the nodes of graph G in a Chimera cross topology.
"""Positions the nodes of graph ``G`` in a Chimera layout with unit cells as crosses.

NumPy (https://scipy.org) is required for this function.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question about NumPy as in drawing.rst

on the x-axis and [-1, 0] on the y-axis.

center : None or array (default None)
Coordinates of the top left corner.

dim : int (default 2)
Number of dimensions. When dim > 2, all extra dimensions are
Number of dimensions. If ``dim`` > 2, all extra dimensions are
set to 0.

Returns
-------
xy_coords : function
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function

If overlapped_embedding is True, then chains in emb may overlap (contain
the same vertices in G), and the drawing will display these overlaps as
If True, chains in the ``emb`` parameter may overlap (contain
the same vertices in ``G``), and the drawing will display these overlaps as
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"the drawing will display these overlaps" --> "the drawing displays these overlaps"

function. If `linear_biases` or `quadratic_biases` are provided,
any provided `node_color` or `edge_color` arguments are ignored.
Parameters in :func:`~networkx.drawing.nx_pylab.draw_networkx`, except for the ``pos`` parameter.
If the ``linear_biases`` or ``quadratic_biases`` parameters are specified,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"If the linear_biases or quadratic_biases parameters are specified" --> This function does not have linear_biases or quadratic_biases parameters, at least in the above descriptions.

Copy link
Contributor Author

@qwriter qwriter Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were present in the original. Perhaps they are referring to the parameters in draw_chimera? Should we point to draw_chimera?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest leaving this bit (for all three topographies) alone in this PR because I suspect the current functionality has problems that will be fixed by this PR. Currently, it seems to me that the node_color and edge_color params are not supported:

G = dnx.chimera_graph(2)
embedding = {"N1": [0, 4, 5], "N2": [1, 6, 7]}
dnx.draw_chimera_embedding(G, embedding, show_labels=True, node_color="r")

returns a TypeError: networkx.drawing.nx_pylab.draw() got multiple values for keyword argument 'node_color' error.

In any case the other PR and this change have conflicts so leaving it untouched will save Kelly work.

"""
draw_embedding(G, chimera_layout(G), *args, **kwargs)


def draw_chimera_yield(G, **kwargs):
"""Draws the given graph G with highlighted faults, according to layout.
"""Draws the given graph ``G`` with highlighted faults.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Draws the given graph G with highlighted faults" --> consistency: other one-line descriptions don't use "given"

The color to use for nodes and edges of G which are not faults.
If unused_color is None, these nodes and edges will not be shown at all.
Color to use for graph ``G``'s nodes and edges which are not faults.
If None, these nodes and edges will not be shown.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"these nodes and edges will not be shown" --> "these nodes and edges are not shown"

Parameters in :class:`.draw_networkx`, except for the ``pos`` parameter.
If the ``linear_biases`` or ``quadratic_biases`` parameters are specified,
the :func:`~networkx.drawing.nx_pylab.draw_networkx` ``node_color``
or ``edge_color`` parameters are ignored.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"If the linear_biases or quadratic_biases parameters are specified" --> This function does not have linear_biases or quadratic_biases parameters, at least in the above descriptions.

Copy link
Contributor

@JoelPasvolsky JoelPasvolsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments on Pegasus functions


Graph H (blue) overlaid on a small Pegasus lattice(yellow nodes and black edges).
Graph H (blue) overlaid on a small Pegasus lattice(yellow nodes and black edges).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for consistency we should probably :code:`` (backtick) the H


Graph H (blue) overlaid on a small Pegasus lattice(yellow nodes and black edges).
Graph H (blue) overlaid on a small Pegasus lattice(yellow nodes and black edges).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a good place to also point out that the layout is rendered in cross format to help users understand what "cross" refers to when used elsewhere (be a good result returned when someone searches "cross")

by an edge in this graph. If given, only couplers between chains
Graph that contains all keys of the ``emb`` parameter as nodes. If specified,
edges of ``G`` are considered interactions if and only if (1) they
exist between two chains of the emb parameter and (2) their keys are connected
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"of the emb parameter" --> "of the emb parameter"

function. If ``linear_biases`` or ``quadratic_biases`` are provided,
any provided ``node_color`` or ``edge_color`` arguments are ignored.
Parameters in :func:`~networkx.drawing.nx_pylab.draw_networkx`, except for the ``pos`` parameter.
If the ``linear_biases`` or ``quadratic_biases`` parameters are specified,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"""
crosses = kwargs.pop("crosses", False)
draw_embedding(G, pegasus_layout(G, crosses=crosses), *args, **kwargs)

def draw_pegasus_yield(G, **kwargs):
"""Draws the given graph G with highlighted faults, according to layout.
"""Draws the given graph ``G`` with highlighted faults.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function. If `linear_biases` or `quadratic_biases` are provided,
any provided `node_color` or `edge_color` arguments are ignored.
Parameters in :func:`~networkx.drawing.nx_pylab.draw_networkx`, except for the ``pos`` parameter.
If the ``linear_biases`` or ``quadratic_biases`` parameters are specified,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -32,31 +32,31 @@


def pegasus_layout(G, scale=1., center=None, dim=2, crosses=False):
"""Positions the nodes of graph G in a Pegasus topology.
"""Positions the nodes of graph ``G`` in a Pegasus topology.

`NumPy <https://scipy.org>`_ is required for this function.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set to 0.

crosses: boolean (optional, default False)
If True, :math:`K_{4,4}` subgraphs are shown in a cross
rather than L configuration.
rather than an L configuration.

Returns
-------
xy_coords : function
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@JoelPasvolsky JoelPasvolsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments on Zephyr functions

If given, only couplers between chains based on this graph are displayed.
Graph that contains all keys of the ``emb`` parameter as nodes. If specified,
the edges of ``G`` are considered interactions if and only if (1) they
exist between two chains of the emb parameter and (2) the keys of the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
exist between two chains of the emb parameter and (2) the keys of the
exist between two chains of the ``emb`` parameter and (2) the keys of the

Adding a Suggested Change so you can see how you commit these without needing to edit yourself

are provided, any provided ``node_color`` or ``edge_color`` arguments are
ignored.
Parameters in :func:`~networkx.drawing.nx_pylab.draw_networkx`, except for the ``pos`` parameter.
If the ``linear_biases`` or ``quadratic_biases`` parameters are specified,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"""
draw_embedding(G, zephyr_layout(G), *args, **kwargs)

def draw_zephyr_yield(G, **kwargs):
"""Draws the given graph G with highlighted faults, according to layout.
"""Draws the given graph ``G`` with highlighted faults, according to the Zephyr layout.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The color to use for nodes and edges of G which are not faults.
If unused_color is None, these nodes and edges will not be shown at all.
Color to use for nodes and edges of ``G`` which are not faults.
If None, these nodes and edges will not be shown.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
If None, these nodes and edges will not be shown.
If None, these nodes and edges are not shown.

Adding a second Suggested Change so you can see how you can collect all such suggestion into a single commit

are provided, any provided ``node_color`` or ``edge_color`` arguments are
ignored.
Parameters in :func:`~networkx.drawing.nx_pylab.draw_networkx`, except for the ``pos`` parameter.
If the ``linear_biases`` or ``quadratic_biases`` parameters are specified,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -31,25 +31,25 @@


def zephyr_layout(G, scale=1., center=None, dim=2):
"""Positions the nodes of graph G in a Zephyr topology.
"""Positions the nodes of graph ``G`` in a Zephyr topology.

`NumPy <https://scipy.org>`_ is required for this function.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[0, 1] on the x-axis and [-1, 0] on the y-axis.

center : None or array (default None)
Coordinates of the top left corner.

dim : int (default 2)
Number of dimensions. When dim > 2, all extra dimensions are
Number of dimensions. If ``dim`` > 2, all extra dimensions are
set to 0.

Returns
-------
xy_coords : function
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@JoelPasvolsky JoelPasvolsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plus remaining open comment from before

dwave_networkx/drawing/chimera_layout.py Outdated Show resolved Hide resolved
@arcondello arcondello merged commit 17a33a6 into dwavesystems:main Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants