PlanarEmbedding.remove_edge() now updates removed edge's neighbors

[mdealencar]

Removing an edge in a PlanarEmbedding requires updating the data dict ('cw' and 'ccw' values) of neighboring edges on both ends. This PR implements this updating.

closes #6796

[dschult]

This looks very good! And fast too.. :)
Could you add a simple test to the tests/test_planarity.py module that removes an edge or two and checks planarity -- similar to however you tracked down this issue.

Thanks!

[mdealencar]

Is that test what you had in mind?

[dschult]

Thanks for that. It should go in the tests folder next to the planarity.py module. So, in this case, networkx/algorithms/tests/test_planarity.py sorry for not being clear.

I also now realize that this creates a difference in the treatment of add_edge and remove_edge. In the current code, add_edge and remove_edge are both handled by the superclass nx.DiGraph. Adding half edges is handled by the subclass PlanarEmbedding. Since this is removing half edges should we create new methods instead of these -- something like: remove_half_edge_cw and remove_half_edge_ccw?

Another question: what happens if removing that edge makes it no longer possible to have a planar graph?

Yet another question: what about removing nodes? Is that not needed?

As an aside, I think these features were not included because they led to headaches -- and a user could recreate the graph with the removed edges if needed. But that was obviously not documented nor enforced.
What should the api be here?

[rossbar]

As an aside, I think these features were not included because they led to headaches -- and a user could recreate the graph with the removed edges if needed. But that was obviously not documented nor enforced.

Perhaps the PlanarEmbedding could be made immutable, at least for adding/removing nodes/edges (attributes should still be okay, right?). If a user wants to modify the embedding, they can create a graph object from the embedding, e.g. H = nx.Graph(my_embedding). It puts more burden on the user in terms of LOC, but at least that way things are explicit - planarity is no longer implied by the type of the object, and the need to re-compute planarity after a change is clear.

[mdealencar]

Perhaps the PlanarEmbedding could be made immutable, at least for adding/removing nodes/edges (attributes should still be okay, right?). If a user wants to modify the embedding, they can create a graph object from the embedding, e.g. H = nx.Graph(my_embedding). It puts more burden on the user in terms of LOC, but at least that way things are explicit - planarity is no longer implied by the type of the object, and the need to re-compute planarity after a change is clear.

I would rather not have PlanarEmbedding immutable. AFAIK, removing edges or nodes from a planar embedding cannot make it not a planar embedding. As long as the structures' internal references are updated correctly, the graph should pass the PlanarEmbedding.check_structure().

I added a .remove_node() method and modified the .remove_edge() to also update the first_nbr node property, which I had overlooked earlier.

That way, only the addition of edges can lead to invalidating the embedding. If there is a need to make PlanarEmbedding more robust to changes in the graph, I'd suggest replacing the add_half_edge...() methods by a single add_edge() that checks locally if planarity is preserved.

[mdealencar]

Is there any chance this PR will be merged?

[dschult]

I'm afraid there is a lot more checking to do before this PR will be merged.
For example, you stated that removing edges does not break the trait of the graph being planar. I agree... :}
But it might break the representation that PlanarEmbedding provides to check that a graph is planar. For example, the code says that one must be careful not to call the method to connect components if the components are already connected, especially if one half edge already exists and you are adding the other half edge. So, what does that imply about breaking a component into two components? Is it enough to check local traits?

You can tell from comments in the code that it was written in a fragile way. As long as you do "normal" things, it will be correct. But it doesn't protect you from yourself. Another example if that it subclasses nx.DiGraph but changes G.is_directed() to return False. That will break lots of code in NetworkX if you try to use it as a networkx graph. This fragility impacts us as maintainers because we have to figure out the impact of seemingly small changes.

Another example is that adding an edge to the graph could easily make it no longer planar. But we have no mechanism for tracking that. This PR will provide G.remove_edge but not make G.add_edge correct (nor block it from happening). Users will be more tempted to use G.add_edge if they are using G.remove_edge somewhere else.

It'd be great to expand on this class to make it editable -- with checks for each edit to ensure that the attribute structure is maintained. Or make the class "frozen" as suggested above. Then people are expected to change the graph in its form as a standard networkx graph. Once changed they can go back to a planar embedding to check traits there. Both would help this code become less "fragile". But the is_directed feature will still make it hard to use/maintain.

Can you convince us that this change does not break the PlanarEmbedding data structure? Are you willing to take on making the class fully editable? Can you describe a use case where making these changes to the code and maintaining them is better for the users than having them use the networkx graph to make graph changes and then convert the new graph to a PlanarEmbedding to check planarity. How slow is that anyway?

I think those are three things I would want to see for this.

[mdealencar]

But it might break the representation that PlanarEmbedding provides to check that a graph is planar.

The representation planarity_state = LRPlanarity(G) only exists within a check_planarity(G) call (i.e. outside the PlanarEmbedding class).

The only structure beyond a DiGraph that a PlanarEmbedding contains is encoded in edge attributes ('cw', 'ccw') and node attribute 'first_nbr'. PlanarEmbedding.check_structure(self) checks the consistency of these attributes plus some global checks based on DiGraph properties only (e.g. both half-edges present, Eulers' formula). This check is distributive over all connected components of the graph (fresh call to nx.connected_components(self) each time), which also clears this:

So, what does that imply about breaking a component into two components? Is it enough to check local traits?

There is no need to worry about the splitting of a component by an edge removal: the planarity code is prepared to deal with multiple components.

Another example is that adding an edge to the graph could easily make it no longer planar. But we have no mechanism for tracking that. This PR will provide G.remove_edge but not make G.add_edge correct (nor block it from happening). Users will be more tempted to use G.add_edge if they are using G.remove_edge somewhere else.

I can include the blocking of PlanarEmbedding.add_edge(). Any hints on which exception to throw?

Another example if that it subclasses nx.DiGraph but changes G.is_directed() to return False. That will break lots of code in NetworkX if you try to use it as a networkx graph. This fragility impacts us as maintainers because we have to figure out the impact of seemingly small changes.

I have the impression that implementing PlanarEmbedding as a nx.Graph would be less of a hassle, but I haven't looked deep enough into all the details. Any valid embedding will have both edges anyway. It would just be a matter of keying ('cw', 'ccw') by source node. The only advantage I see of using DiGraph is that the algorithm for building it can add all half-edges from a node at a time, before dealing with the corresponding complementary half-edges.

It'd be great to expand on this class to make it editable -- with checks for each edit to ensure that the attribute structure is maintained.

I'm proposing making it editable only for removals. I believe the PR enables removals in a non-breaking way, which is an improvement over the current status. For additions, it would require implementing a local planarity test (I can take a shot at it, but I am not sure how complicated that is). For the time being, additions would throw an exception.

Can you convince us that this change does not break the PlanarEmbedding data structure?

I hope the above was convincing enough.

Are you willing to take on making the class fully editable?

I'd have to investigate a bit further for that (and whether the use of nx.Graph would be an improvement).

Can you describe a use case where making these changes to the code and maintaining them is better for the users than having them use the networkx graph to make graph changes and then convert the new graph to a PlanarEmbedding to check planarity.

I cannot say what is better for the users, but I'm using my PR in a project and find it useful. I even add edges (which I know that will maintain the planarity). I'm working with Euclidean Delaunay triangulations, but I need to make small adjustments to them as a spanning tree is built over the nodes. It would impact negatively my algorithm if I were to recreate a PlanarEmbedding after each change.

The current state is buggy, so I think this PR is an improvement, but I agree those changes add to the maintenance burden.

How slow is that anyway?

Not sure, but it's not a trivial procedure.

[dschult]

We talked about this in a community meeting recently and we are OK with having remove_edge/node added to the class, but we'd really like it if the other morphing methods would raise exceptions. We are worried that people will see that they can remove and edge and then try to add an edge.

Could you add methods for the following that simply raise an exception stating that this is not allowed for a PlanarEmbedding? The methods are (obtained from the function frozen which turns off all of these).

    G.add_node
    G.add_nodes_from
    G.add_edge
    G.add_edges_from
    G.add_weighted_edges_from

If that's too much for now let us know and we'll find a way to get it in.

[Edited: if we then in the future want to make one of these work, we still can. But it completes this improvement in PlanarEmbedding]
Thanks for all of this!

[mdealencar]

Could you add methods for the following that simply raise an exception stating that this is not allowed for a PlanarEmbedding?

G.add_edge
G.add_edges_from
G.add_weighted_edges_from

I had to override those above by overriding the base class' __init__(), since they are necessary if the instance is created with incoming_graph_data (tests do that a lot).

As for these:

G.add_node
G.add_nodes_from

Addition of nodes do not pose a risk of invalidating the PlanarEmbedding and are required by LRPlanarity.lr_planarity(), which is called if one tries to create a PlanarEmbedding by calling check_planarity(nx.Graph()). So I didn't touch them.

Let me know if there is anything missing.

[dschult]

Thanks!
This looks ready to me -- but one last pass through lead me to two requests.

• use nx.NetworkXError rather than NetworkXError in the remove methods.
• add simple tests of the forbidden functions.

(see below)

[rossbar]

Hmm unfortunately this is very difficult to review due to something going very wrong with your fork - when I try to check out your branch it looks like a ton of stuff has been deleted, including the pyproject.toml which makes it impossible to install.

I'm not sure exactly what happened, but if there's a commit pre-rebase attempts that still has all of the proposed changes it might be worth trying to roll back to that!

[rossbar]

I may be missing something here, but IMO it'd be more idiomatic to override these methods directly instead of assigning them to a hidden/forbidden method, e.g.

def add_edge(self, *args, **kwargs):
    raise NotImplemented("Use `add_half_edge` to add edges to a PlanarEmbedding")

# same for the other methods

I'd also vote for using NotImplemented instead of NetworkXError, which I think fits slightly better and will automatically resolve the "tests passing for the wrong reason" issue.

[mdealencar]

Thanks for reviewing, @rossbar

I rebased again this branch and now I think I stacked on top of PR #7202 correctly.

I may be missing something here, but IMO it'd be more idiomatic to override these methods directly instead of assigning them to a hidden/forbidden method, e.g.

The constructor of DiGraph (PlanarEmbedding's parent class) uses self.add_edge*(). This happens via
delegation of incoming_graph_data to functions in convert.py. If I override add_edge*() in the class definition, those functions will fail. I avoided that by extending DiGraph's constructor and overriding add_edge*() after incoming_graph_data is processed.

I think that in order to use the idiomatic approach you suggested, the PlanarEmbedding's constructor would have to do something like that if it receives graph data:

#  not exactly what we need, since it returns a new object
LRPlanarity(nx.Graph(incoming_graph_data)).lr_planarity() 

Though this would make it very hard to create invalid embeddings for the test instances.

I'd also vote for using NotImplemented instead of NetworkXError, which I think fits slightly better and will automatically resolve the "tests passing for the wrong reason" issue.

The "tests passing for the wrong reason" issue was already solved by only using the context pytest.raises() for the actual functionality under test.

Let me know if the difficulties with checking this PR out remains.

[rossbar]

Ah, good point - thanks for the explanation, that makes sense. It sounds like there's opportunity for a deeper refactor here, but I'm inclined not to let that be a blocker for this PR!

The "tests passing for the wrong reason" issue was already solved by only using the context pytest.raises() for the actual functionality under test.

True, though I still feel like NotImplemented is a better-fitting exception, as this has to do with methods on the Python object we're forbidding, rather than anything dealing with graphs (in the abstract sense), where a NetworkX-flavored exception would be expected. This is very subjective though and also not a blocker!

[mdealencar]

I've changed the exception raised by the forbidden methods to NotImplementedError (tests updated accordingly).

[dschult]

Thanks for all of this! I think separating into two PRs really helped get it across the line.

I approve this PR (and have one suggestion below that doesn't matter except for cleanliness)
Yay!

[dschult]

Should we remove this comment that doesn't seem to be relevant anymore?

[mdealencar]

done

[rossbar]

This LGTM, thanks again @mdealencar for all the continued work on the PlanarEmbedding!