Fix undefined variables for color maps #233
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #233 +/- ##
==========================================
+ Coverage 75.51% 75.92% +0.41%
==========================================
Files 31 31
Lines 2193 2181 -12
==========================================
Hits 1656 1656
+ Misses 537 525 -12 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Since you're already touching linear_biases/quadratic_biases here, would you mind fixing their mutable default argument?
Also, adding (at least some smoke) tests for draw_qubit_graph that verify the fix would be nice.
| 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')) |
There was a problem hiding this comment.
Is a change in semantics intended here? Consider the case when user supplies cmap=None.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Actually, the same question applies to all of these.
There was a problem hiding this comment.
|
I'll put in some tests |
kevinchern
force-pushed
the
bug/undefined_maps
branch
from
44e8944 to
4fe8ffc
Compare
|
|
||
| @unittest.skipUnless(np and plt, "No numpy or matplotlib") | ||
| class TestDrawing(unittest.TestCase): | ||
| @unittest.skipUnless(_display, " No display found") |
There was a problem hiding this comment.
Not sure you need this if using non-interactive matplotlib backend. Btw, which backend is used here? Hopefully not interactive..
There was a problem hiding this comment.
I added this for consistency with existing tests, e.g., https://github.com/dwavesystems/dwave-networkx/blob/main/tests/test_pegasus_layout.py#L68-L104
| # 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', None) or plt.get_cmap('coolwarm') | ||
| vmin = kwargs.pop('vmin', None) or -1 * vmag |
There was a problem hiding this comment.
or might not be what you want here. E.g. if user explicitly sets vmin=0, you override it to -vmag.
There was a problem hiding this comment.
When I say "here", I mean in this line, and a few other lines below.
For this issue #232