Skip to content

Fix typos and styling. #226

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.

Sign up for GitHub

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

Merged
arcondello merged 6 commits into from
Mar 20, 2023
Merged

Fix typos and styling. #226

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fcbd1a4
Fix typos and styling.
qwriter Feb 3, 2023
1ad8d98
Implement Joel and Alex C's feedback.
qwriter Mar 14, 2023
c164faa
Implement Joel's feedback.
qwriter Mar 15, 2023
373201e
Update dwave_networkx/drawing/chimera_layout.py
qwriter Mar 16, 2023
54a54c0
Remove note about NumPy requirement.
qwriter Mar 16, 2023
f02010d
Merge remote-tracking branch 'origin/copyedit_drawing' into copyedit_…
qwriter Mar 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions docs/reference/drawing.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
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

Expand All @@ -20,7 +20,10 @@ Chimera Graph Functions
:toctree: generated/

chimera_layout
chimera_node_placer_2d
draw_chimera
draw_chimera_embedding
draw_chimera_yield

Example
~~~~~~~
Expand Down Expand Up @@ -69,8 +72,9 @@ Pegasus Graph Functions
.. autosummary::
:toctree: generated/

draw_pegasus
draw_pegasus
draw_pegasus_embedding
draw_pegasus_yield
pegasus_layout
pegasus_node_placer_2d

Expand Down Expand Up @@ -111,10 +115,11 @@ Zephyr Graph Functions
.. autosummary::
:toctree: generated/

draw_zephyr
draw_zephyr
draw_zephyr_embedding
draw_zephyr_yield
zephyr_layout
zephyr_node_placer_2d

Example
~~~~~~~
Expand Down
139 changes: 70 additions & 69 deletions dwave_networkx/drawing/chimera_layout.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
#
# ================================================================================================
"""
Tools to visualize Chimera lattices and weighted graph problems on them.
Tools to visualize :term:`Chimera` lattices and weighted :term:`graph` problems on them.
"""

import networkx as nx
Expand All @@ -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

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

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

Copy link
Contributor

@JoelPasvolsky JoelPasvolsky Mar 13, 2023

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

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

"in a Chimera cross layout"?

Copy link
Contributor

@JoelPasvolsky JoelPasvolsky Mar 13, 2023

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

@JoelPasvolsky JoelPasvolsky Mar 15, 2023

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

"""Position the nodes of graph ``G`` in a Chimera cross topology.
Copy link
Contributor

@JoelPasvolsky JoelPasvolsky Feb 7, 2023

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.


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

@JoelPasvolsky JoelPasvolsky Mar 15, 2023

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


Parameters
----------
G : NetworkX graph
Should be a Chimera graph or a subgraph of a
Chimera graph. If every node in G has a `chimera_index`
attribute, those are used to place the nodes. Otherwise makes
a best-effort attempt to find positions.
:term:`Chimera` :term:`graph` or :term:`subgraph` of a
Chimera graph. If every node in ``G`` has a ``chimera_index``
attribute, the node position in the ``chimera_index``
attribute is used to place each node. Otherwise,
a best-effort attempt is made to find the node positions.

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

@JoelPasvolsky JoelPasvolsky Feb 7, 2023

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

@JoelPasvolsky JoelPasvolsky Feb 7, 2023

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 •
edited
Loading

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

@JoelPasvolsky JoelPasvolsky Mar 13, 2023

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 •
edited
Loading

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

@qwriter qwriter Mar 14, 2023

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.

set to 0.

Returns
-------
pos : dict
A dictionary of positions keyed by node.
Dictionary of positions keyed by node.

Examples
--------
Expand Down Expand Up @@ -106,13 +107,13 @@ def chimera_layout(G, scale=1., center=None, dim=2):


def chimera_node_placer_2d(m, n, t, scale=1., center=None, dim=2):
"""Generates a function that converts Chimera indices to x, y
coordinates for a plot.
"""Generate a function that converts Chimera indices to x- and
y-coordinates for a plot.

Parameters
----------
m : int
Number of rows in the Chimera lattice.
Number of rows in the :term:`Chimera` lattice.

n : int
Number of columns in the Chimera lattice.
Expand All @@ -121,22 +122,21 @@ def chimera_node_placer_2d(m, n, t, scale=1., center=None, dim=2):
Size of the shore within each Chimera tile.

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]
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
set to 0.

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

@JoelPasvolsky JoelPasvolsky Mar 15, 2023

Choose a reason for hiding this comment

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

Function

A function that maps a Chimera index (i, j, u, k) in an
(m, n, t) Chimera lattice to x,y coordinates such as
used by a plot.
Function that maps a Chimera index ``(i, j, u, k)`` in an
``(m, n, t)`` Chimera lattice to x- and y-coordinates.

"""
import numpy as np
Expand Down Expand Up @@ -190,30 +190,30 @@ def _xy_coords(i, j, u, k):


def draw_chimera(G, **kwargs):
"""Draws graph G in a Chimera cross topology.
"""Draw graph ``G`` in a Chimera cross topology.

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

@JoelPasvolsky JoelPasvolsky Feb 7, 2023

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

in the ``linear_biases`` and ``quadratic_biases`` parameters.

Parameters
----------
G : NetworkX graph
Should be a Chimera graph or a subgraph of a Chimera graph.
:term:`Chimera` :term:`graph` or a :term:`subgraph` of a Chimera graph.

linear_biases : dict (optional, default {})
A dict of biases associated with each node in G. Should be of
form {node: bias, ...}. Each bias should be numeric.
A dict of biases associated with each node in ``G`` and of
the form ``{node: bias, ...}``. Each bias is numeric.
Copy link
Contributor

@JoelPasvolsky JoelPasvolsky Feb 7, 2023

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


quadratic_biases : dict (optional, default {})
A dict of biases associated with each edge in G. Should be of
form {edge: bias, ...}. Each bias should be numeric. Self-loop
A dict of biases associated with each edge in ``G`` and of
the form ``{edge: bias, ...}``. Each bias is numeric. Self-loop
edges (i.e., :math:`i=j`) are treated as linear biases.

kwargs : optional keywords
See networkx.draw_networkx() for a description of optional keywords,
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

@arcondello arcondello Feb 3, 2023

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

@qwriter qwriter Feb 7, 2023

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

@JoelPasvolsky JoelPasvolsky Feb 7, 2023

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

If the ``linear_biases`` or ``quadratic_biases`` parameters are specified,
then the :func:`~networkx.drawing.nx_pylab.draw_networkx` ``node_color``
or ``edge_color`` parameters are ignored.

Examples
--------
Expand All @@ -230,87 +230,88 @@ def draw_chimera(G, **kwargs):


def draw_chimera_embedding(G, *args, **kwargs):
"""Draws an embedding onto the chimera graph G, according to layout.
"""Draw an embedding onto the chimera graph ``G``, according to the Chimera layout.
Copy link
Contributor

@JoelPasvolsky JoelPasvolsky Feb 7, 2023

Choose a reason for hiding this comment

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

What does ", according to the Chimera layout" add to comprehension here?

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.

Are you saying that we can delete this phrase because it is in the Chimera layout by context?
Also, can we delete "in a Chimera cross topology" in other places. Also, what is a "cross topology"?

Copy link
Contributor

@JoelPasvolsky JoelPasvolsky Mar 13, 2023

Choose a reason for hiding this comment

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

Yes, I don't understand what the "according to layout" clause adds to the preceding clause here. Perhaps there's good intention but insufficiently clear writing behind the existing sentence.

I can't answer the general question of deleting something in other places without knowing what those places say.

For "cross" you might be referring to cross layout drawing as opposed to column layout. Whether or not that info is essential or not, depends on the local context.


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

@JoelPasvolsky JoelPasvolsky Feb 7, 2023

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

list. If the ``embedded_graph`` parameter is not None, then only display the couplers between
chains with intended couplings according to the ``embedded_graph`` parameter.

Parameters
----------
G : NetworkX graph
Should be a Chimera graph or a subgraph of a Chimera graph.
:term:`Chimera` :term:`graph` or a :term:`subgraph` of a Chimera graph.

emb : dict
A dict of chains associated with each node in G. Should be
of the form {node: chain, ...}. Chains should be iterables
of qubit labels (qubits are nodes in G).
A dict of chains associated with each node in ``G`` and
of the form ``{node: chain, ...}``. Chains are iterables
of qubit labels (qubits are nodes in ``G``).

embedded_graph : NetworkX graph (optional, default None)
A graph which contains all keys of emb as nodes. If specified,
edges of G will be considered interactions if and only if they
exist between two chains of emb if their keys are connected by
an edge in embedded_graph
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

@JoelPasvolsky JoelPasvolsky Feb 7, 2023

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

@JoelPasvolsky JoelPasvolsky Feb 7, 2023

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.


show_labels: boolean (optional, default False)
If show_labels is True, then each chain in emb is labelled with its key.
If True, then each chain in the ``emb`` parameter is labelled with its key.

chain_color : dict (optional, default None)
A dict of colors associated with each key in emb. Should be
of the form {node: rgba_color, ...}. Colors should be length-4
tuples of floats between 0 and 1 inclusive. If chain_color is None,
each chain will be assigned a different color.
A dict of colors associated with each key in the ``emb`` parameter and
of the form ``{node: rgba_color, ...}``. Colors must be length-4
tuples of floats between 0 and 1, inclusive. If None,
then each chain will be assigned a different color.

unused_color : tuple (optional, default (0.9,0.9,0.9,1.0))
The color to use for nodes and edges of G which are not involved
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

@JoelPasvolsky JoelPasvolsky Feb 7, 2023

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 None, then these nodes and edges will not be shown.
Copy link
Contributor

@JoelPasvolsky JoelPasvolsky Feb 7, 2023

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


overlapped_embedding: boolean (optional, default False)
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, then chains in the ``emb`` parameter may overlap (contain
the same vertices in ``G``), and the drawing will display these overlaps as
Copy link
Contributor

@JoelPasvolsky JoelPasvolsky Mar 15, 2023

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"

concentric circles.

kwargs : optional keywords
See networkx.draw_networkx() for a description of optional keywords,
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.
If the ``linear_biases`` or ``quadratic_biases`` parameters are specified,
Copy link
Contributor

@JoelPasvolsky JoelPasvolsky Mar 15, 2023

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 •
edited
Loading

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

@JoelPasvolsky JoelPasvolsky Mar 15, 2023

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.

then the :func:`~networkx.drawing.nx_pylab.draw_networkx` ``node_color``
or ``edge_color`` parameters are ignored.
"""
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.
"""Draw the given graph ``G`` with highlighted faults, according to the Chimera layout.

Parameters
----------
G : NetworkX graph
The graph to be parsed for faults
:term:`Graph` to be parsed for faults.

unused_color : tuple or color string (optional, default (0.9,0.9,0.9,1.0))
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, then these nodes and edges will not be shown.

fault_color : tuple or color string (optional, default (1.0,0.0,0.0,1.0))
A color to represent nodes absent from the graph G. Colors should be
length-4 tuples of floats between 0 and 1 inclusive.
Color to represent nodes that are absent from graph ``G``. Colors must be
length-4 tuples of floats between 0 and 1, inclusive.

fault_shape : string, optional (default='x')
The shape of the fault nodes. Specification is as matplotlib.scatter
marker, one of 'so^>v<dph8'.
Shape of the fault nodes. The shapes are the same as those specified for
`Matplotlib markers <https://matplotlib.org/stable/api/markers_api.html#module-matplotlib.markers>`_.

fault_style : string, optional (default='dashed')
Edge fault line style (solid|dashed|dotted|dashdot)
Line style for fault edges. The line style can be any of the following values:
``'solid'``, ``'dashed'``, ``'dotted'``, ``'dashdot'``.

kwargs : optional keywords
See networkx.draw_networkx() for a description of optional keywords,
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.
If the ``linear_biases`` or ``quadratic_biases`` parameters are specified,
then the :func:`~networkx.drawing.nx_pylab.draw_networkx` ``node_color``
or ``edge_color`` parameters are ignored.
Copy link
Contributor

@JoelPasvolsky JoelPasvolsky Mar 15, 2023

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.

"""
try:
assert(G.graph["family"] == "chimera")
Expand Down
Loading