networkx / networkx-metis

NetworkX Addon to allow graph partitioning with METIS
Other
79 stars 21 forks source link

Fix for redirecting of stdout problem #14

Closed OrkoHunter closed 9 years ago

OrkoHunter commented 9 years ago

Fixes errors concerning StringIO instance of sys.stdout.

@ysitu This line

    msg = unicode(tmp.read(), sys.stdout.encoding)

needs to be changed too.

AttributeError: StringIO instance has no attribute 'encoding'
ysitu commented 9 years ago

msg = unicode(tmp.read(), sys.stdout.encoding)

How about str(tmp.read(), 'ascii')?

OrkoHunter commented 9 years ago

How about str(tmp.read(), 'ascii')?

str() does not take two arguments. Can't we use decode() or encode() functions here?

ysitu commented 9 years ago

decode() is the right choice.

OrkoHunter commented 9 years ago

And so, it works! :)

OrkoHunter commented 9 years ago

We need to update the directory structure of the existing files in the master. Should I add one commit here and change the name of PR or make a separate one after this goes in?

ysitu commented 9 years ago

Make a separate PR.

OrkoHunter commented 9 years ago

That would be possible after this goes in, to avoid merge conflicts.

ysitu commented 9 years ago

Are you able to test this PR?

OrkoHunter commented 9 years ago

@ysitu Yes, and it works. I have a local setup which does not uses the namespace packing for installation. So, I test the changes there manually.

Regarding the automated script and Travis CI, that'd be working as soon as the namespace installation starts working for networkx-metis, which seems to be a little brainstorming for me.

ysitu commented 9 years ago

Do you mean that you have a test case that forces METIS to generate an error message, and you can verify that the error message is correctly captured?

OrkoHunter commented 9 years ago

I have fetched all the files from the current master plus files currently split into three PRs (#3 #4 #14), at one place, in one test directory and then I run

$ python setup.py build_clib

followed by

$ python setup.py build_ext --inplace

And for this particular PR, I run nosetests which verify the tests written under tests/test_metis.py and then I manually try partitioning a graph by importing networkx.

In the current situation, after this PR

$ nosetests                                               [master] 10:52:53
....
----------------------------------------------------------------------
Ran 4 tests in 0.096s

OK
ysitu commented 9 years ago

Those test cases do not really exercise the code paths that deal with exceptions.

OrkoHunter commented 9 years ago

But those tests were failing before this patch. They pass now.

ysitu commented 9 years ago

The tests passing does not mean that any output from METIS is correctly captured. We need to make sure that we relay some meaningful error messages to the user when something goes wrong.

OrkoHunter commented 9 years ago

We need to make sure that we relay some meaningful error messages to the user when something goes wrong.

That means more test cases.

OrkoHunter commented 9 years ago

What are some possible situations when things would go wrong? Partitioning directed or multi graphs are covered, nparts > 0 covered, what more could possibly go wrong? We might need another PR for tests. As much as I understand, the problem of StringIO instance of std.stdout is covered, which is why this PR was created.

ysitu commented 9 years ago

I think that you can put some invalid values into MetisOptions, e.g., niter <= 0. In options.c there is a CheckParams function. You can find the expected error message there.

OrkoHunter commented 9 years ago

Does that mean adding something like test_MetisOptions in test_metis.py?

diff --git a/tests/test_metis.py b/tests/test_metis.py
index 337477d..a50a691 100644
--- a/tests/test_metis.py
+++ b/tests/test_metis.py
@@ -2,7 +2,7 @@ import itertools
 import nose.tools

 import _metis
-
+import _types

 def make_cycle(n):
     xadj = list(range(0, 2 * n + 1, 2))
@@ -68,3 +68,10 @@ class TestMetis(object):
             sorted(map(sorted,
                        [[(sep[0] + i) % n for i in range(1, n // 2)],
                         [(sep[1] + i) % n for i in range(1, n // 2)]])))
+
+    def test_MetisOptions(self):
+        n = 16
+        xadj, adjncy = make_cycle(n)
+        objval, part = _metis.part_graph(
+                xadj, adjncy, 2, options=_types.MetisOptions)
+            

...followed by assertions of errors listed in CheckParams of options.c

I don't have much idea about using options in part_graph and other functions.

ysitu commented 9 years ago

You create a MetisOptions object and set its niter property to a negative value.

OrkoHunter commented 9 years ago

Simply this?

    def test_MetisOptions(self):
        options = _types.MetisOptions
        options.niter = -1

How should I invoke MetisOptions in the functions like part_graph?

OrkoHunter commented 9 years ago

Well, we should be doing this over #3.

ysitu commented 9 years ago

You should do

n = 16
xadj, adjncy = make_cycle(n)
options = types.MetisOptions()
options.niter = -1
assert_raises_regexp(MetisError, '<error message regex>',
                     _metis.part_graph, xadj, adjncy, 2, options=options)
OrkoHunter commented 9 years ago

For the following,

    def test_MetisOptions(self):
        n = 16
        xadj, adjncy = make_cycle(n)
        options = _types.MetisOptions()
        options.niter = -1
        nose.tools.assert_raises_regexp(_types.MetisError, 'Input Error: Incorrect niter.',
                             _metis.part_graph, xadj, adjncy, 2, options=options)

I get

Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/hunter/GSoC/2015/test/tests/test_metis.py", line 78, in test_MetisOptions
    _metis.part_graph, xadj, adjncy, 2, options=options)
AssertionError: MetisError not raised
ysitu commented 9 years ago

Was an exception raised at all?

OrkoHunter commented 9 years ago

I guess no. It's independent of the error string 'Input Error: Incorrect niter.'

OrkoHunter commented 9 years ago

Error is raised if niter < -1. This one passes the test and the MetisError is being raised in it.

    def test_MetisOptions(self):
        n = 16
        xadj, adjncy = make_cycle(n)
        options = _types.MetisOptions(niter=-2)
        nose.tools.assert_raises_regexp(_types.MetisError, 'Input Error: Incorrect niter.',
                             _metis.part_graph, xadj, adjncy, 2, options=options)
OrkoHunter commented 9 years ago

Rebased branch. PR updated with