Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix and add test to ensure proper handling of merges by adjust_partition function #7

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix and add test to ensure proper handling of merges by adjust_partition function #7

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3f0c4d6
BF: add test of and fix bug for merges in modularity adjust_partition…
Jun 21, 2013
406dfdb
RF: Added additional test to test_adjust_modularity
Jun 23, 2013
c4e7bf6
RF: Adjustments to simple_net to ensure a merge is always optimal sol…
Jun 23, 2013
286d972
BF: Fix test in test_adjust_partition
Jun 24, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 20 additions & 8 deletions brainx/modularity.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,12 +388,12 @@ def node_update(self, n, m1, m2):
#Compute the change in modularity
return (e1[m1]-a1[m1]**2) + (e1[m2]-a1[m2]**2) - mod_old

def compute_node_update(self, n, m1, m2):
def compute_node_update(self, node, m1, m2):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the more explicit names!

"""Moves a single node within or between modules

Parameters
----------
n : node identifier
node : node identifier
The node that will be moved from module m1 to module m2
m1 : module identifier
The module that n used to belong to.
Expand All @@ -408,14 +408,14 @@ def compute_node_update(self, n, m1, m2):
n1 = self.index[m1]
n2 = self.index[m2]

node_moved_mods = {0: n1 - set([n]),1: n2 | set([n])}

node_moved_mods = {0 : n1 - set([node]), 1 : n2 | set([node])}
# Before we overwrite the mod vectors, compute the contribution to
# modularity from before the change
e1 = [0,0]
a1 = [0,0]
e0, a0 = self.mod_e, self.mod_a

# The values that change: _edge_info with arguments will update the e,
# a vectors only for the modules in index
e1, a1 = self._edge_info(e1, a1, node_moved_mods)
Expand All @@ -424,8 +424,9 @@ def compute_node_update(self, n, m1, m2):
delta_q = ( (e1[0]-a1[0]**2) + (e1[1]-a1[1]**2)) - \
( (e0[m1]-a0[m1]**2) + (e0[m2]-a0[m2]**2) )


#print n,m1,m2,node_moved_mods,n1,n2
return node_moved_mods, e1, a1, -delta_q, n, m1, m2
return node_moved_mods, e1, a1, -delta_q, node, m1, m2

def apply_node_update(self, n, m1, m2, node_moved_mods, e_new, a_new):
"""Moves a single node within or between modules
Expand Down Expand Up @@ -1393,7 +1394,7 @@ def adjust_partition(g, partition, max_iter=None):
iterations = 0
max_iter = max_iter or np.inf
best_modularity = partition.modularity()

while nodes and no_improvement < 10 and iterations <= max_iter:
moves = []
move_modularity = []
Expand All @@ -1404,13 +1405,24 @@ def adjust_partition(g, partition, max_iter=None):
moves.append((n, node_map[n], p))
M = -partition.compute_node_update(n, node_map[n], p)[3]
move_modularity.append(M)

(n, p0, p1) = moves[np.argmax(move_modularity)]

split_modules, e_new, a_new = partition.compute_node_update(n, p0, p1)[:3]
partition.apply_node_update(n, p0, p1, split_modules, e_new, a_new)
node_map[n] = p1
nodes.remove(n)

# Perform adjustments after merge instances
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the rest of the code depend on partitions being labeled contiguously? If so, isn't that rather what we should fix?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, currently the code depends on this contiguous structure because of its utilization of dictionaries rather than lists. At present, the switch from dictionaries to lists would be very involved, as this dependency is engrained in the entire package and how some of the users work with the final output it produces. That said, we are still planning to work on it, but in the mean time might as well feel comfortable knowing that this section of the code is working properly.
Either way, I think the partitions from which adjust_partition selects and attempts to implement should be updated after a merge occurs still.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, just to make sure I understand: aren't these just they keys of the dictionary, and can't we simply use an OrderedDictionary or iterate over the keys to look at the different modules? Apologies if I'm completely off-base, I'm not up to date with the internal representation used here.

if not P == set(range(len(partition))):
# Reset the module labels in the initial partition
P = set(range(len(partition)))
# Recreate the dict that maps nodes to their partition labels
node_map = {}
for p in P:
for node in partition.index[p]:
node_map[node] = p

print '[%d/%d] -> %.4f' % (len(nodes), L, partition.modularity())

M = partition.modularity()
Expand Down
15 changes: 15 additions & 0 deletions brainx/tests/simple_net.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
0 1
0 4
0 5
1 2
1 3
2 3
3 4
4 5
5 6
6 7
6 8
6 9
7 8
7 9
8 9
34 changes: 22 additions & 12 deletions brainx/tests/test_modularity.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -738,17 +738,28 @@ def test_apply_node_move():


def test_adjust_partition():
e = np.loadtxt(os.path.join(os.path.dirname(__file__), 'jazz.net'),
# Check that adjust_partition returns best output
e1 = np.loadtxt(os.path.join(os.path.dirname(__file__), 'jazz.net'),
skiprows=3, dtype=int)[:, :2] - 1
g = nx.Graph()
g.add_edges_from(e)

p0 = mod.newman_partition(g)
p1 = mod.adjust_partition(g, p0, max_iter=6)

npt.assert_(p0 > 0.38)
npt.assert_(p1 > 0.42)

g1 = nx.Graph()
g1.add_edges_from(e1)

g1_p0 = mod.newman_partition(g1)
g1_p1 = mod.adjust_partition(g1, g1_p0, max_iter=6)

npt.assert_(g1_p0 > 0.38)
npt.assert_(g1_p1 > 0.42)

# Check that adjust_partition handles merges properly
e2 = np.loadtxt(os.path.join(os.path.dirname(__file__), 'simple_net.txt'),
dtype=int)
g2 = nx.Graph()
g2.add_edges_from(e2)

g2_p0 = {0: {0,1,2}, 1: {3,4,5,6}, 2: {7,8,9}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be made into a graph partition for test to run properly

g2_p1 = mod.adjust_partition(g2, g2_p0)

npt.assert_(g2_p0 > 0.39)

def test_empty_graphpartition():
g = nx.Graph()
Expand All @@ -769,7 +780,6 @@ def test_badindex_graphpartition():
npt.assert_raises(TypeError, mod.GraphPartition, g, {0: g.nodes()})
npt.assert_raises(ValueError, mod.GraphPartition, g, {0:set(g.nodes()[:-1])})
npt.assert_raises(TypeError, mod.GraphPartition, g, g.nodes())



if __name__ == "__main__":
npt.run_module_suite()