Fix draw_networkx_nodes with list node_shape and add regression test #8363
Fix draw_networkx_nodes with list node_shape and add regression test #8363dschult merged 3 commits intonetworkx:mainfrom
Conversation
dschult
left a comment
There was a problem hiding this comment.
Straightforward improvement. Thanks for the test too!
|
Looks like there are whitespace changes requested (you can do them manually or use The test base failures are not related to this PR. Its a cpython error that is being worked on. |
0f05ba4 to
c54fc88
Compare
| node_color="red", # scalar color (supported) | ||
| node_shape=shapes, # list of shapes – this used to be buggy |
There was a problem hiding this comment.
I don't think this is quite right - this is already allowed and will pass on main. The unit test should fail on main and pass with the fix. The easiest way to do this is to adhere more closely to the example in the OP of the issue, which uses colors=["red"]*5 (i.e. a list of scalars).
There was a problem hiding this comment.
whoops... missed that!
Yes -- this test should be using a list for each of node_color and node_shape.
Good catch!
[Removed some mixed up comments...]
We do need to change this test. Sorry for the confusion.
The problem we are solving here appears from the original post in that issue, not from my apparently incorrect summary further down.
There was a problem hiding this comment.
OK... This PR does fix a problem with the current code.
When node_shapes is a list, no exception is raised. But the resulting collection is empty. No nodes satisfy the condition node_shape == shape so nothing is added to the collection.
We need to update the test to check that nodes actually has (some number of) elements in it.
See the suggestions below.
We should also remove the subplots and fig/ax stuff. No ax=ax is needed on the function call.
networkx/drawing/tests/test_pylab.py
Outdated
| ) | ||
|
|
||
| # Should get a valid PathCollection back (same behavior as with numpy arrays) | ||
| assert isinstance(nodes, mpl.collections.PathCollection) |
There was a problem hiding this comment.
What we need here is to check that the "length of the PathCollection" is >0. So I think the following assert is needed.
| assert isinstance(nodes, mpl.collections.PathCollection) | |
| assert len(nodes.get_offsets()) > 0 |
Note that we do have to assert on this length here. Even if the len is not 5 it should be > 0. So the comment following this can be removed.
And the subplots and ax=ax stuff can be removed too. We only need to call the function.
Unfortunately this stuff is necessary, but for a very indirect reason... if we don't include explicit fig/ax creation in the tests, then it is done implicitly inside the Of course, this detail isn't documented anywhere! |
|
Ahh -- interesting point about ax and fig creation needing to be explicit so we are sure to test only the parts of So, now we just need to change the assert and update/remove the comment after it. |
|
In the sense that this is a bug-fix. It might be good to include it in 3.6.1 too. |
|
I went ahead and pushed the suggested change (test now fails on main) and tweaked the comments. |
Fixed #8350
This pull request fixes the behavior of
draw_networkx_nodeswhennode_shapeis passed as a Python list.
Motivation
Previously,
draw_networkx_nodescorrectly handlednode_shapevalue such as"o",numpy.ndarrayof shapes,but when
node_shapewas a plain Python list, the comparison