Linear time implementation of Modular Decomposition for Undirected Graphs

[lokeshj1703]

This is aimed at providing linear time implementation for finding modular decomposition of undirected graphs, fixing the currently broken Sage's graph modular decomposition.

CC: @dimpase @dcoudert

Component: graph theory

Keywords: modular decomposition, gsoc2017

Author: Lokesh Jain

Branch: 6688bac

Reviewer: David Coudert, Dima Pasechnik

Issue created by migration from https://trac.sagemath.org/ticket/23487

[dimpase]

Changed keywords from modular decomposition to modular decomposition, GSOC 2017

[dimpase]

comment:1

It will use the implementation from https://github.com/lokeshj1703/Undirected-Modular-Decomposition

[dimpase]

Description changed:

--- 
+++ 
@@ -1 +1 @@
-This ticket is related to GSOC 2017 and is aimed at providing linear time implementation for finding modular decomposition of undirected graphs.
+This is aimed at providing linear time implementation for finding modular decomposition of undirected graphs, fixing the currently broken Sage's graph modular decomposition.

[dimpase]

Changed keywords from modular decomposition, GSOC 2017 to modular decomposition, gsoc2017

[lokeshj1703]

Branch: u/jlokesh/23487

[lokeshj1703]

New commits:

340580e

trac #23487: added modular decomposition module + removed old one

[lokeshj1703]

Commit: 340580e

[dcoudert]

comment:4

Dear Lokesh,

why have you removed file modular_decomposition.pyx ? You should put it back for the moment. There are dependencies and so I'm unable to compile sage with your patch.

Many improvements are needed in your code that we will slowly address. Dima is certainly more expert than me in this. For instance:

• Comments must be in 80 column mode. Some editors like emacs ease the formatting.
• Prefer # Nested module to #Nested module to improve readability
• You can also insert some empty lines in some code blocks to improve readability
• Returns True if ... -> Return True if ...
• You will also have to insert some TESTS: and EXAMPLES: blocks to illustrate the behavior of the methods, in particular for modular_decomposition, and for testing cases that could lead to errors.

[dcoudert]

Reviewer: David Coudert, Dmitrii Pasechnik

[dimpase]

comment:5

Replying to @dcoudert:

Dear Lokesh,

why have you removed file modular_decomposition.pyx ? You should put it back for the moment. There are dependencies and so I'm unable to compile sage with your patch.

this should be trivial to fix. Let's remove it now for good.

[dimpase]

Changed commit from 340580e to dd540d6

[dimpase]

comment:6

this is the updates needed for Sage to build and run. modular_decomposition doctests in graph.py fail due to some format incompatibility of output of the new implementation. I'll leave it to Lokesh to fix, as well as the David's comments, naturally.

---

New commits:

dd540d6

updates to allow sage build. doctests in graph.py still fail

[dimpase]

Changed branch from u/jlokesh/23487 to public/gsoc17_t23487

[lokeshj1703]

comment:7

Sorry for the late reply. I have started working on the updates. I will push the commit shortly.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

83387cb

trac #23487: improved readability

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from dd540d6 to 83387cb

[lokeshj1703]

comment:9

I have added a commit for improved readability in the code. This commit addresses first four issues pointed out by David. I will soon add a commit for including examples, test cases and for handling failure of doctests.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 83387cb to 0a5a504

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

0a5a504

trac #23487: added tests, examples + fixed doctests

[lokeshj1703]

comment:11

I have added the commit for fixing the doctests and added the Tests and Examples for modular_decomposition function. Should I add the examples for other functions as well? Because most of the functions are called from modular_decomposition itself.

For the tests for modular_decomposition I have added the graph from Marc Tedder research paper and an example from the wikipedia modular decomposition page. Further I have added a series graph (Tetrahedral Graph).

[f29946bc-ee7b-48cd-9abc-3445948c551d]

comment:13

Is #13744 a duplicate of this?

[dimpase]

comment:14

Replying to @jm58660:

Is #13744 a duplicate of this?

Yes, the latter can be closed as won't fix, with the fixing done here.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 0a5a504 to 1e6c92f

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

62e5487	Merge branch 'public/gsoc17_t23487' of trac.sagemath.org:sage into moddec
1e6c92f	rebased over curr. beta and removed an obsolete NOTE

[dimpase]

comment:16

As this is to be included in 8.1, the branch should be based on the current beta - as I just did in 1e6c92f.

[dimpase]

comment:17

I will clean up the import statements from sage.*. Specifically, there is a handy import_statements() command in Sage:

sage: import_statements('Graph')
from sage.graphs.graph import Graph

importing from sage.all instead is much less efficient.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

646507e

fixed import and name/address in copyright

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 1e6c92f to 646507e

[dimpase]

comment:19

Is there a function to create the quotient graph with the vertices being the max. modules?

Please also make it clear which functions are a part of the main algorithm, and which ones are only used for testing.

Please also add more TEST and EXAMPLE; e.g. the functions to compute mu certainly need tests. In general, all the functions, except 1-liners, will need tests and docs.

[dcoudert]

comment:20

Some comments:

• in class Queue, method get could return a ValueError. Isn't it more appropriate to directly raise an error?
• in form_module, variable flag can be removed to use while True:
• in either_connected_or_not_connected, use graph.has_edge(u, v) instead of v in graph.neighbors(u)
• method number_subtree could be an internal method of recursively_number_cocomponents. Only used here and number_subtree is the recursive part
• in assembly: while not len(root[1]) == 1: -> while len(root[1]) != 1: ?
• in update_comp_num, instead of tree you could use child
• in get_vertex_in: improve the description since the output block gives more details than the 1 line decription. Also, you don't need a recursive method here. You could rewrite it as

while tree[0].node_type != NORMAL:
    tree = tree[1][0]
return tree[1][0]

• if left_index - 1 >= 0: -> if left_index >= 1: ?
• in is_component_connected, you don't need to convert explicitly neighbor to set. The isdisjoint accepts a list as input.
• method recurse_component should be an internal method of get_vertices
• in method refine: for e in graph.edges_incident(v): -> for u in graph.neighbor_iterator(v): seems more appropriate. In fact, you can directly build the set x as x = {u for u in graph.neighbor_iterator(v) if vertex_dist[u] != vertex_dist[v]}. You will then have to update the definition ofxin the inputs ofmaximal_subtrees_with_leaves_in_x`
• in maximal_subtrees_with_leaves_in_x, instead of using root[1][-1][0] and root[1][-2][1], why not creating first node a, filling it, add it to root[1], then creating node b, filling it and adding it to root[1] ?
• Add one line description to methods create_prime_node, etc.
• the test methods (test_modular_decomposition, etc.) should be move at the end of the file after a separator like #=====================...

The writing style in sagemath is to start methods with a 1 line description of the functionality, then a blank line, then a longer description of the functionality if needed. So

def recursively_number_cocomponents(tree, cocomp_num, by_type):
    """
    Recursively number the nodes in the (co)components.

    If the tree node_type is same as by_type then cocomp_num is incremented
    before assigning to the subtree else entire tree is numbered by cocomp_num.

    INPUTS:
    """

It's a long patch to review...

[lokeshj1703]

comment:21

Replying to @dimpase:

Is there a function to create the quotient graph with the vertices being the max. modules?

Currently there is no function which creates a quotient graph with the vertices being the maximal modules. I can create such a function though?

Please also make it clear which functions are a part of the main algorithm, and which ones are only used for testing.

David has asked me to move the test functions at the end after a separator, it will solve this problem.

Please also add more TEST and EXAMPLE; e.g. the functions to compute mu certainly need tests. In general, all the functions, except 1-liners, will need tests and docs.

I will definitely add more TESTS and EXAMPLES. It might take some time as each function requires some arguments mostly a tree or a module. I will need to input that manually.

Thanks for fixing the import and copyright!

[lokeshj1703]

comment:22

Replying to @dcoudert:

Thanks for reviewing the patch! I will make the changes and add a commit soon.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 646507e to 433ee5e

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

433ee5e

trac #23487: improved code + documentation

[dcoudert]

comment:24

It is much better now. Some more comments.

• on the left hand side, you don't need to write [a,b] = [1,2]. You can directly write a,b = [1,2]

• You can permute variables without intermediate variable as a,b = b,a

• in method __str__, you don't need variable string. You can directly return the result.

• in the EXAMPLES block of modular_decomposition, you must put :: at the end of the sub blocks instead of :. This is for the html documentation.

• Some of the examples in the TESTS block of modular_decomposition could be in the EXAMPLES block. Also, can you add sub blocks with indication on the origin of the tested graphs ?

• Then, can you have tests for bad inputs in modular_decomposition, like directed graph, etc. i.e., the raise error stuff

• I have an identation issue with the """ before the code

• in compute_mu_for_co_component: You don't need the intermediate variable mu_for_co_component.

• in compute_mu_for_component: why not using range(source_index-1, -1, -1) and returning the first found value ?

• method recursive_print_md_tree could be an internal method of print_md_tree. Also, instead of using level, you could directly pass a string as parameter, recursive_print_md_tree(tree, level + " "). Last, use print("{}{}".format(level,str(root[0]))) for Python 3 compatibility.

[lokeshj1703]

comment:25

Replying to @dcoudert:

• I have an identation issue with the """ before the code

I could not understand which """ you are talking about? Is it the one in the beginning? Is the indentation issue in the doc build?

• in compute_mu_for_component: why not using range(source_index-1, -1, -1) and returning the first found value ?

  if mu_for_component == root[1][index] and \

Suppose [c01, c02, c03, source, c1, c2, c3] is the forest. If mu value for c2 is c03 then c01 and c02 must be connected to c2. Therefore the iteration from 0 is required.

I have incorporated the other changes and will add a commit for it.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

72d87e8

improvements in the code and documentation

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 433ee5e to 72d87e8

[dcoudert]

comment:27

Replying to @lokeshj1703:

Replying to @dcoudert:

• I have an identation issue with the """ before the code

I could not understand which """ you are talking about? Is it the one in the beginning? Is the indentation issue in the doc build?

The one before if graph._directed.

Note that it is better to use if graph.is_directed() ;)

[dcoudert]

comment:28

When you add a link to a wikipedia page, the nice way to do it is: :wikipedia:`Modular_decomposition`.

[lokeshj1703]

comment:29

Replying to @dcoudert:

The one before if graph._directed.

Note that it is better to use if graph.is_directed() ;)

Found it. :) I will include these changes in the next commit along with TESTS and EXAMPLES for other functions.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 72d87e8 to 6f7af1e

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

6f7af1e

trac #23487: added TESTS and EXAMPLES

[2c00b582-cfe9-43b9-8124-fec470060704]

comment:31

Out of curiosity, have you considered using collections.dequeue instead of making your own (inefficient) Queue class?

https://wiki.python.org/moin/TimeComplexity

[dcoudert]

comment:32

I was not aware of this data structure. It's certainly what we need here.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

92c1de0	Merge branch 'develop' into moddec
1ff6668	Merge branch 'public/gsoc17_t23487' of trac.sagemath.org:sage into moddec
ab14394	removed "print " from tests/examples

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 6f7af1e to ab14394

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. New commits:

caedc65

trac #23487: Used colloection.deque instead of Queue

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from ab14394 to caedc65

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from caedc65 to 12f3f15