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.

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

Fix undefined variables for color maps #233

Merged
merged 4 commits into from
Jan 18, 2024
+109 −64
Merged

Fix undefined variables for color maps #233

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c57d80a
Fix undefined variables for color maps
kevinchern Nov 16, 2023
7acafe3
Fix mutable default args
kevinchern Nov 21, 2023
4fe8ffc
Fix kwargs edge case, clean code, and add tests
kevinchern Nov 21, 2023
ab7c3c9
Handle edge cases for default kwargs
kevinchern Nov 22, 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
43 changes: 16 additions & 27 deletions dwave_networkx/drawing/qubit_layout.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,24 +102,6 @@ def draw_qubit_graph(G, layout, linear_biases={}, quadratic_biases={},
if edgelist is None:
edgelist = G.edges()

# since we're applying the colormap here, matplotlib throws warnings if
# we provide these arguments and it doesn't use them.
if linear_biases:
cmap = kwargs.pop('cmap', None)
vmin = kwargs.pop('vmin', None)
vmax = kwargs.pop('vmax', None)

if quadratic_biases:
edge_cmap = kwargs.pop('edge_cmap', None)
edge_vmin = kwargs.pop('edge_vmin', None)
edge_vmax = kwargs.pop('edge_vmax', None)

if cmap is None:
cmap = plt.get_cmap('coolwarm')

if edge_cmap is None:
edge_cmap = plt.get_cmap('coolwarm')

# any edges or nodes with an unspecified bias default to 0
def edge_color(u, v):
c = 0.
Expand All @@ -143,17 +125,16 @@ def node_color(v):
# the range of the color map is shared for nodes/edges and is symmetric
# around 0.
vmag = max(max(abs(c) for c in node_color), max(abs(c) for c in edge_color))
if vmin is None:
vmin = -1 * vmag

if vmax is None:
vmax = vmag

if edge_vmin is None:
edge_vmin = -1 * vmag
# since we're applying the colormap here, matplotlib throws warnings if
# we provide these arguments and it doesn't use them.
cmap = kwargs.pop('cmap', plt.get_cmap('coolwarm'))
Copy link
Member

Choose a reason for hiding this comment

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

Is a change in semantics intended here? Consider the case when user supplies cmap=None.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The semantics should be the same. In the previous commit, we have

if linear_biases:
    cmap = kwargs.pop('cmap', None)
...
if cmap is None:
    cmap = plt.get_cmap('coolwarm')

which would have the same behaviour. The issue is if linear_biases was never supplied, then the second conditional throws an error as cmap is undefined.

Copy link
Member

Choose a reason for hiding this comment

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

No, previously if the function was called with cmap=None (explicitly set), you would end up with cmap = plt.get_cmap('coolwarm') (assuming it doesn't fail).

Now, if user explicitly sets cmap=None, the final value will be cmap = None.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AH, OK I misunderstood.

vmin = kwargs.pop('vmin', -1 * vmag)
vmax = kwargs.pop('vmax', vmag)

if edge_vmax is None:
edge_vmax = vmag
edge_cmap = kwargs.pop('edge_cmap', plt.get_cmap('coolwarm'))
edge_vmin = kwargs.pop('edge_vmin', -1 * vmag)
edge_vmax = kwargs.pop('edge_vmax', vmag)
Copy link
Member

Choose a reason for hiding this comment

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

Actually, the same question applies to all of these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with https://github.com/dwavesystems/dwave-networkx/pull/233/files#diff-344ce7a800a7c461ff087cf0103c3ba81b6af1935497c0b179431a811faee391R134-R140


if linear_biases and quadratic_biases:
global_vmin = min(edge_vmin, vmin)
Expand Down Expand Up @@ -189,6 +170,14 @@ def node_color(v):
if ax is None:
ax = fig.add_axes([0.01, 0.01, 0.98, 0.98])

if linear_biases and not quadratic_biases:
kwargs['edge_vmin'] = edge_vmin
kwargs['edge_vmax'] = edge_vmax
kwargs['edge_cmap'] = edge_cmap
if quadratic_biases and not linear_biases:
kwargs['vmin'] = vmin
kwargs['vmax'] = vmax
kwargs['cmap'] = cmap
draw(G, layout, ax=ax, nodelist=nodelist, edgelist=edgelist, **kwargs)


Expand Down