Rewrite test_metis.py to test exposed functions #44

OrkoHunter · 2015-07-16T05:01:56Z

Fixes #26

I'm not sure whether it's the best testing method but I've used a nx.complete_graph(16) and tested the results against what I got on my computer.

As the results are generated by algorithms based on approximations, they depend upon the platform and compiler used.

OrkoHunter · 2015-07-16T05:11:45Z

For nxmetis.partition(G, 4)

msvc on Windows - (96, [[1, 2, 3, 9], [0, 4, 5, 6], [7, 8, 10, 15], [11, 12, 13, 14]])
gcc on Linux   -  (96, [[1, 2, 3, 10], [4, 5, 6, 7], [8, 9, 12, 14], [0, 11, 13, 15]])

For nxmetis.vertex_separator(G)

msvc on Windows - ([2, 4, 5, 6, 7, 9, 10], [0, 1, 3, 8, 11, 12, 13, 14, 15], []) 
gcc on Linux   -  ([1, 2, 4, 6, 7, 9, 13], [0, 3, 5, 8, 10, 11, 12, 14, 15], [])

OrkoHunter · 2015-07-16T09:26:49Z

It appears that we are not the only one facing this problem.

http://stackoverflow.com/questions/31299159/metis-different-results-on-different-os
http://stackoverflow.com/questions/29397370/getting-non-unique-results-from-metis-when-different-compiler-is-used

OrkoHunter · 2015-07-16T09:31:07Z

I think the good way here to test METIS should be that if it returns couples of nested lists or tuple (the way we expect) the tests should pass unless there is some error returned. Eventually we can't be sure for what METIS is going to return for a particular operation.

ysitu · 2015-07-16T15:44:39Z

nxmetis/tests/test_metis.py

 from nxmetis import exceptions
 from nxmetis import _metis
 from nxmetis import types

+G = nx.complete_graph(16)  # An unweighted complete Graph made of 16 nodes


Move this into to setUp() function under TestMetis:

def setUp(self): self.G = nx.complete_graph(16)

Also, consider using a graph whose nodes are not integers. That way we can verify that we are returning nodes to users instead of their indices in enumeration.

ysitu · 2015-07-16T15:49:12Z

It is true that we should tolerate variation in METIS output. But we should still verify the invariants. For example, when we do graph partitioning, we should verify that the returned partitions do not overlap.

ysitu · 2015-07-16T18:11:38Z

nxmetis/tests/test_metis.py

+        node_list = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j',
+                     1, 2, 3, 4, 5, 6]
+        self.G = nx.Graph()
+        self.G.add_edges_from(list(itertools.combinations(node_list, 2)))


I suggest that you use a cycle graph. That gives you some more interesting invariants to check. For example, when you partition it, you should expect to get some contiguous chains of nodes.

OrkoHunter · 2015-07-19T03:10:13Z

nxmetis/tests/test_metis.py

+        j = list(partition)
+        nose.tools.assert_true(seq_in_seq(list(partition)[1][0], self.node_list*2))
+#       nose.tools.assert_true(seq_in_seq(list(partition)[1][1], self.node_list*2))
+#       ['a', 'b', 5, 6] ?? 


OrkoHunter · 2015-07-19T03:15:51Z

I made some changes regarding the test-the-continuous-chains-from-partition. I'll try to finish it later today.

ysitu · 2015-07-19T05:35:47Z

nxmetis/tests/test_metis.py

+    def setUp(self):
+        self.node_list = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j',
+                     1, 2, 3, 4, 5, 6]
+        class OrderedGraph(nx.Graph):


I do not think that using an ordered graph helps. We do not know how METIS handles node ordering internally.

The results produced by METIS varies across different platforms even for the same input. Using OrderedGraph helps us to detect the "chain" which must be the output of partitioning a cycle graph. This is sure a narrow pathway. If we use a random node ordering with usual graph then it's not seemingly possible to verify it.

Let's consider what we can (reasonably) do if we have a cycle graph of 16 nodes whose identities we do not even know.

node_nested_dissection: This function is about finding a node elimination order that reduces fill (think efficient sparse Cholesky factorization). We cannot do much other than verifying set{G} == set{result}.

partition: Let's partition into say four parts. It is reasonable to assume, despite without guarantee, that METIS will return four consecutive segments. We can verify that each of those are nonempty, contiguous and disjoint, and their union contains the same nodes as G.

vertex_separator: Again, it is reasonable to assume in spite of lack of guarantee that METIS will return a separator of exactly two nodes with the remaining nodes forming two contiguous chains. If we consider the two separator nodes two singleton chains, we can verify their correctness the same way we do for partition (perhaps ignoring the nonemptiness condition).

So far, I think we are good with testing node_nested_dissection. We cannot do much than verifying that they generate equal sets.

Regarding partition, now I think we can verify the continuity without the use of OrderedGraph. After all we just have to know the order in G.nodes().

Thanks for the suggestion regarding vertex_separator. I'll try to implement it very soon.

ysitu · 2015-07-20T15:36:29Z

nxmetis/tests/test_metis.py

+        self.G = nx.Graph()
+        edges = [(self.node_list[i], self.node_list[i+1])
+                 for i in range(len(self.node_list) - 1)]
+        edges.append((6, 'a'))  # To complete the cycle


G.add_path(node_list) G.add_edge(node_list[-1], node_list[0])

OrkoHunter · 2015-07-20T19:59:42Z

appveyor.yml

@@ -68,5 +68,3 @@ test_script:
  - "pushd %TEMP%"
  - "nosetests -v nxmetis.tests.test_metis"

-  # Move back to the project folder
-  - "popd"


This was causing the build failure even when the tests would pass. Piggyback it here or a new PR/commit?

I think it should be in one of the fixes for #41

ysitu · 2015-07-21T04:14:33Z

nxmetis/tests/test_metis.py

+        nose.tools.ok_(set(self.node_list) & set(node_ordering) ==
+                       set(self.node_list))
+        nose.tools.ok_(set(self.node_list) & set(node_ordering) ==
+                       set(node_ordering))


Did you notice that you are checking just set(node_list) == set(node_ordering) in a very convoluted way? You are also missing the case where there may be duplicates in node_ordering. You can do this instead:

assert_equal(len(G), len(node_ordering)) assert_equal(set(G), set(node_ordering))

Aah! I don't know what was I thinking at that time. Changing it.

ysitu · 2015-07-22T06:21:17Z

nxmetis/tests/test_metis.py

+        # When we choose one node from one part of the partitioned Graph,
+        # It must be adjacent to one or more of the nodes in the same part.
+        # This is to verify the continuity of the chain of nodes.
+        parts = list(partition)[1]  # List containing partitioned node lists


partition is a tuple. You should not need list().

ysitu · 2015-07-22T06:27:53Z

nxmetis/tests/test_metis.py

+        nose.tools.ok_(list(bisection)[0][0] not in list(bisection)[1])
+        nose.tools.ok_(list(bisection)[0][0] not in list(bisection)[2])
+        nose.tools.ok_(list(bisection)[0][1] not in list(bisection)[1])
+        nose.tools.ok_(list(bisection)[0][1] not in list(bisection)[2])


This is getting hard to read. Use

sep, part1, part2 = nxmetis.vertex_separator(self.G)

and refer to the three return values by name.

OrkoHunter · 2015-07-22T06:28:01Z

Last five comments addressed.

OrkoHunter · 2015-07-22T06:38:05Z

I've made the changes and rebased it because of #47. We can wait for a green tick on appveyor but that might take long because of the pending builds.

ysitu · 2015-07-22T06:46:15Z

nxmetis/tests/test_metis.py

+        nose.tools.ok_(sep[0] not in part1)
+        nose.tools.ok_(sep[0] not in part2)
+        nose.tools.ok_(sep[1] not in part1)
+        nose.tools.ok_(sep[1] not in part2)


You need to first check that len(sep) is two. You also need to verify that sep[0] is different from seq[1].

Yes. Added them L#78 and L#77

Rewrite test_metis.py to test exposed functions

ysitu reviewed Jul 16, 2015
View reviewed changes

OrkoHunter force-pushed the test-change branch from 1c7b845 to 3f6a893 Compare July 16, 2015 16:22

ysitu reviewed Jul 16, 2015
View reviewed changes

OrkoHunter force-pushed the test-change branch from 3f6a893 to a97b9ed Compare July 19, 2015 03:09

OrkoHunter reviewed Jul 19, 2015
View reviewed changes

OrkoHunter force-pushed the test-change branch 2 times, most recently from 7c09cb1 to 1db586a Compare July 19, 2015 03:14

ysitu reviewed Jul 19, 2015
View reviewed changes

OrkoHunter force-pushed the test-change branch 3 times, most recently from 06cb103 to 42735f5 Compare July 20, 2015 15:21

ysitu reviewed Jul 20, 2015
View reviewed changes

OrkoHunter force-pushed the test-change branch 4 times, most recently from f222fd4 to b1d8e59 Compare July 20, 2015 19:46

OrkoHunter reviewed Jul 20, 2015
View reviewed changes

OrkoHunter force-pushed the test-change branch 4 times, most recently from 8fd3674 to aa0142f Compare July 21, 2015 00:35

ysitu reviewed Jul 21, 2015
View reviewed changes

OrkoHunter force-pushed the test-change branch from aa0142f to c39f853 Compare July 21, 2015 13:00

ysitu reviewed Jul 22, 2015
View reviewed changes

OrkoHunter force-pushed the test-change branch from c39f853 to 5dea0b6 Compare July 22, 2015 06:27

ysitu reviewed Jul 22, 2015
View reviewed changes

OrkoHunter force-pushed the test-change branch 2 times, most recently from 95de94e to 5c2d1a6 Compare July 22, 2015 06:34

ysitu reviewed Jul 22, 2015
View reviewed changes

OrkoHunter force-pushed the test-change branch from 5c2d1a6 to a784940 Compare July 22, 2015 07:42

Rewrite test_metis.py to test exposed functions

f737bbd

OrkoHunter force-pushed the test-change branch from a784940 to f737bbd Compare July 22, 2015 07:45

ysitu added this to the networkx-metis-1.0 milestone Jul 22, 2015

ysitu added a commit that referenced this pull request Jul 22, 2015

Merge pull request #44 from OrkoHunter/test-change

b898c25

Rewrite test_metis.py to test exposed functions

ysitu merged commit b898c25 into networkx:master Jul 22, 2015

OrkoHunter deleted the test-change branch July 22, 2015 15:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite test_metis.py to test exposed functions #44

Rewrite test_metis.py to test exposed functions #44

OrkoHunter commented Jul 16, 2015

OrkoHunter commented Jul 16, 2015

OrkoHunter commented Jul 16, 2015

OrkoHunter commented Jul 16, 2015

ysitu Jul 16, 2015

ysitu Jul 16, 2015

ysitu commented Jul 16, 2015

ysitu Jul 16, 2015

OrkoHunter Jul 19, 2015

OrkoHunter commented Jul 19, 2015

ysitu Jul 19, 2015

OrkoHunter Jul 20, 2015

ysitu Jul 20, 2015

OrkoHunter Jul 20, 2015

ysitu Jul 20, 2015

OrkoHunter Jul 20, 2015

OrkoHunter Jul 20, 2015

ysitu Jul 21, 2015

OrkoHunter Jul 21, 2015

ysitu Jul 22, 2015

ysitu Jul 22, 2015

OrkoHunter commented Jul 22, 2015

OrkoHunter commented Jul 22, 2015

ysitu Jul 22, 2015

OrkoHunter Jul 22, 2015

Rewrite test_metis.py to test exposed functions #44

Rewrite test_metis.py to test exposed functions #44

Conversation

OrkoHunter commented Jul 16, 2015

OrkoHunter commented Jul 16, 2015

OrkoHunter commented Jul 16, 2015

OrkoHunter commented Jul 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ysitu commented Jul 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OrkoHunter commented Jul 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OrkoHunter commented Jul 22, 2015

OrkoHunter commented Jul 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment