Add clear edges method as a method to be frozen by nx.freeze#6190
Add clear edges method as a method to be frozen by nx.freeze#6190dschult merged 2 commits intonetworkx:mainfrom AdamWRichardson:bugfix-clear-edges-not-frozen
Conversation
rossbar
left a comment
There was a problem hiding this comment.
Thanks @AdamWRichardson , it does indeed seem that clear_edges not being included in the "frozen" methods is an oversight and the tests look good as well (with a minor suggestion).
| def test_node_attributes_are_still_mutable_on_frozen_graph(self): | ||
| G = nx.freeze(self.G) | ||
| node = G.nodes[0] | ||
| node["node_attribute"] = True | ||
| assert node["node_attribute"] == True |
There was a problem hiding this comment.
The tests look good, but let's use a fresh graph instance instead of the class instance to prevent potential crosstalk with other tests, e.g.:
| def test_node_attributes_are_still_mutable_on_frozen_graph(self): | |
| G = nx.freeze(self.G) | |
| node = G.nodes[0] | |
| node["node_attribute"] = True | |
| assert node["node_attribute"] == True | |
| def test_node_attributes_are_still_mutable_on_frozen_graph(self): | |
| G = nx.freeze(nx.path_graph(3)) | |
| G.nodes[0]["node_attribute"] = True | |
| assert node["node_attribute"] == True |
The same for the edge attr test as well.
There was a problem hiding this comment.
I think the suggestion is missing a line to define node. But the basic idea is to refrain from freezing self.G because that will likely be used by other tests. Instead create a new graph to freeze -- like nx.path_graph(3).
Thanks for this!
There was a problem hiding this comment.
Great, thanks for the review and comments and I have now made the relevant changes!
Has the team thought of using pytest fixtures to ensure each test is isolated?
There was a problem hiding this comment.
OK... now I think I'm confusing myself... :)
This is a test class with a setup_method. So self.G should be created fresh and clean for each test method. I think I am getting confused because we are switching tests (slowly over time) from class methods to functions, using pytest fixtures where appropriate. The test class system provides some of the fixture-like features. And the details of when a test class calls setup_method vs setup vs ??other options?? escape me sometimes.
But if I understand correctly, it should be fine to use self.G in a test class that creates self.G in a setup method.
@rossbar here's a chance for you to state the case for fixtures vs classes. :)
There was a problem hiding this comment.
This is a test class with a setup_method
Nice catch - this is indeed correct; my comment was based on the (incorrect) notion that G was defined as a class attribute. This is the pattern that of course leads to crosstalk between tests as class attributes modified in any test method will remain modified in any subsequent tests. This pattern does appear in the test suite (though pytest-randomly helps weed them out) but @dschult is correct that it is not relevant here.
Sorry for the walls of text @AdamWRichardson - none of these comments are blockers for your change!
rossbar
left a comment
There was a problem hiding this comment.
LGTM, thanks for the fix + tests @AdamWRichardson !
networkx#6190) * Add clear edges method to the list of methods to be frozen by the nx.freeze function * Change tests to create new graph instead of using class attribute
networkx#6190) * Add clear edges method to the list of methods to be frozen by the nx.freeze function * Change tests to create new graph instead of using class attribute
…freeze function
nx.freezewas freezing most of the attributes of a graph but didn't freeze theclear_edgesmethod. This PR adds that method into the list of methods to be frozen.A different design choice would be to re-implement the freeze method to call into a
__freeze__method on the graph. This would allow subclasses to modify the freeze method based on their needs and would also make any future ommisions like this easier to fix in subclasses. However since this is my first time contributing I went for a minimal changes.Happy to have comments and suggestions on this!