Fix output of is_chordal for empty graphs

[navyagarwal]

Closes #6562

For graphs with number of nodes <= 3, is_chordal should return True. I have added a special condition to the code for the same.

Also added a test case that checks if True is returned when an empty graph is passed.

Though, I am confused about a particular test case -

self_loop_G = nx.Graph()
self_loop_G.add_edges_from([(1, 1)])
cls.self_loop_G = self_loop_G

with pytest.raises(nx.NetworkXError, match="Input graph is not chordal"):
       nx.is_chordal(self.self_loop_G)

Since the graph has only 2 nodes, shouldn't it return True?

[PurviChaurasia]

This is where my test case was failing :/ Glad you caught it!

[navyagarwal]

This is where my test case was failing :/ Glad you caught it!

I think we could either remove the test case, or correct it with the right output.

[PurviChaurasia]

I think before doing either, it's important to understand why it is happening. @dschult perhaps you can help us out?

[dschult]

Nice work folks!

I have a question for you: A chordal graph is defined only for undirected graphs with no self-loops (as far as I can tell). So, should a graph with a self-loop automatically make is_chordal return False?
Even more corner-case-y: should a graph with 2 nodes that has a self-loop return True?

What is the right way to handle selfloops? It seems like their existence violates the standard definitions of chordal, so should we raise an exception like we do with directed graphs? I'd prefer to extend the definition of chordal to graphs with self-loops if we can find a way that works. I think the restriction to considering cycles with 4 or more edges was done to allow triangles. We're trying to build triangles by cutting up the bigger cycles via chords.

Now, how would self-loops fit into that world? Maybe they just don't fit. Or maybe we just ignore them.

If they don't fit, we should probably raise a NetworkXError (with a better message than the existing one which just says it is not chordal, and it doesn't say anything about self-loops... that could be confusing to someone who doesn't realize their large graph as a single self-loop edge).

If we ignore self-loops on the reasoning that they don't cause simple cycles that can be made shorter by replacing part of the cycle with a chord, does everything work out? We thus define a chordal graph as being a graph with no simple cycles of 4 or more edges that don't contain a chord. The "simple cycles" part means the self-loops can exist, but they don't participate in any cycles. Would this re-definition lead to trouble with other chordal-related functions?

I think the "ignore self-loops" approach would simply involve removing lines 332 and 333. But it should also add lines to the documentation (probably just two lines per function: the line saying selfloops are ignored and a blank line separating it from the others). And we could turn on that selfloop test again, only have it check that the selfloop doesn't affect the outcome rather than checking that it raises an error.

Thoughts?

[PurviChaurasia]

From my short experience, self loops have always been a tricky corner case. However I think "ignoring" them would be a better situation rather than trying to come up with a mid way approach. This is again what @rossbar previously said in one of my issues that making the function more restrictive is always better than having the possibility of it behaving in an unknown manner.

[PurviChaurasia]

Thanks a lot for the explanation @dschult. They have always given a clear understanding of what's happening in the function and where we are going wrong :)

[PurviChaurasia]

Line 332 and 333 should be commented out as we're planning on ignoring the case with self loops. I think adding a small explanation in the docstring would also help clear out any confusion.

[navyagarwal]

@dschult I have made the changes for ignoring the case of self-loops here.

But I am interested in having a conversation about why the Graph class in Networkx is allowed to have self-loops. In a lot of sources, particularly CLRS, I have read that self loops are forbidden in an undirected graph. It is also referred to as a simple graph. Whereas both self-loops and parallel edges are allowed in a Multigraph.

Wouldn't it make more sense to have a graph class in Networkx that does not allow self-loops? Since this conversation has a much broader scope, maybe I can move it to the Discussions tab

[MridulS]

But I am interested in having a conversation about why the Graph class in Networkx is allowed to have self-loops. In a lot of sources, particularly CLRS, I have read that self loops are forbidden in an undirected graph. It is also referred to as a simple graph. Whereas both self-loops and parallel edges are allowed in a Multigraph.

Wouldn't it make more sense to have a graph class in Networkx that does not allow self-loops? Since this conversation has a much broader scope, maybe I can move it to the Discussions tab

A quick point here is that the networkx Graph is not a simple graph (and it doesn't really pretend to be one) and there are a lot of competing definitions specifically for self loops (especially for a lot of algorithms). If a user wants to have a simple graph class they can create a graph subclass that adds the constraints. Adding another base class expands the user API by a lot, so we need to be cautious about it :)

But yes, this does sound like a discussion topic :)

[dschult]

The Graph class is based on dict-of-dict-of-dict storage. That doesn't rule out self-loops, and it is somewhat costly to e.g. check every edge addition to make sure self-loops are not added. Instead we chose to let the user police themselves. If they want a simple_graph they can either 1) check the edges they are adding before adding them, or 2) add edges and then remove self-loops when they are ready to do a calculation that requires a simple graph.

We do try to make it easy to work with simple graphs. For example, nx.number_of_selfloops() can be used to check whether it is a simple graph. And eliminating self-loops is fairly easy: G.remove_edges_from(nx.selfloop_edges(G))

But the short answer to your question "why is Graph not a simple graph" is that we avoid checking every edge that is added this way. And we try to help the users who need a simple graph (which Graph can be -- just doesn't have to be).

[navyagarwal]

The Graph class is based on dict-of-dict-of-dict storage. That doesn't rule out self-loops, and it is somewhat costly to e.g. check every edge addition to make sure self-loops are not added.

But the short answer to your question "why is Graph not a simple graph" is that we avoid checking every edge that is added this way.

This is the information I was hoping to get! I wanted to understand why this design choice was made, got it, thank you!

[rossbar]

This was discussed at the most recent community meeting and the (loose) consensus was that it should be the responsibility of the user to know that the input graph does not contain self-loops. This fact should be documented of course, so I think this PR is on the right track!

[rossbar]

IMO this is an improvement over the current behavior - i.e. users will no longer get the StopIteration error for an empty graph, aligning with current behavior for graphs with fewer than 4 nodes. Thanks @navyagarwal