networkx / networkx-metis

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

Added python wrappers #3

Closed OrkoHunter closed 9 years ago

OrkoHunter commented 9 years ago

This adds python wrappers and tests.

OrkoHunter commented 9 years ago

I had to rename types.py to avoid conflicts with the python built-in module types.

chebee7i commented 9 years ago

from . import types

This used to raise Attempted relative import from non-package error. Due to a namespace package, I guess we may not put __init__.py on the top.

That's a really unappealing "feature" of namespace packages. It seems like any sufficiently complicated namespace package would want the option to do relative imports, rather than having to hardcode the package name in absolute imports. The use case of a namespace package consisting of more than one module isn't discussed in PEP 420

And, would this even work in Python 3? Python 3 does not allow implicit relative imports at all. Also, note that the way to create namespace packages differs in 3.x than it does in 2.x.

Though I haven't dug into this, it seems like a namespace package is potentially more complicated then doing the simple approach:

# networkx/metis/__init__.py

from __future__ import absolute_import

# this is an absolute import to .../site-packages/python2.7/networkx_metis
from networkx_metis import *

Then users use it as: import networkx.metis as metis.

ysitu commented 9 years ago

How about not using relative import at all? We know where this package will land after installation.

OrkoHunter commented 9 years ago

@ysitu That is good. Maybe we can change all the imports before the final release of networkx-metis. As of now, it's under development, we should just keep it so.

chebee7i commented 9 years ago

Yeah that's fine. Have you guys looked into how to do this in a 2/3 compatible way? Would we just use pkgutil or would we use setuptools?

chebee7i commented 9 years ago

And I do realize this is minutiae compared to the larger compilation issues you are focusing on now. So feel free to ignore my comments until they become relevant.

OrkoHunter commented 9 years ago

@chebee7i No, I'm reading things about it in particular and I think setuptools would be the way to go. However, we'll discuss about it.

chebee7i commented 9 years ago

Are you guys depending on six. It looks like it provides reraise which let's you do this without having to code up _exec.

http://stackoverflow.com/a/14505093

ysitu commented 9 years ago

@chebee7i That is actually a good suggestion. Using six in this case is certainly a big plus in code readability.

OrkoHunter commented 9 years ago

@chebee7i Thanks for the suggestion. six seems to solve our problem in one another way. The exec statement was replaced with an exec(...) function in Python 3. Replace:

exec code in globals, locals

With:

import six
six.exec_(code, globals, locals)

Source : https://github.com/ipython/ipython/wiki/IPEP-4:-Python-3-Compatibility#syntax-changes

Should I remove _exec and use it?

chebee7i commented 9 years ago

Doesn't reraise do exactly what you are intending with those lines in _convert_exceptions? If I understand it correctly, you won't need to call six.exec_, six.reraise will handle that for you.

OrkoHunter commented 9 years ago

So, if we are not using six.exec_(code, globals, locals), then I'm a little unclear about the syntax with six.raise(). What would that be?

Note: six.exec_ doesn't need any other _exec function either

chebee7i commented 9 years ago

I haven't really looked into it, but my first guess is that something like the following could work:

   def _convert_exceptions(func, *args, **kwargs):
       try:
           return func(*args, **kwargs)
       except catch_types or () as e:
           exc = sys.exec_info()
       except Exception as e:
           if catch_types is not None:
               raise
           exc = sys.exec_info()
       six.reraise(convert_type, convert_type(exc[1]), exc[2])
   return _convert_exceptions
OrkoHunter commented 9 years ago

@chebee7i That seems to work just fine. Thanks!

chebee7i commented 9 years ago

It imports/compiles or it passes all the tests? :)

OrkoHunter commented 9 years ago

@chebee7i Yes it compiles. Tests are not passing anyway.

OrkoHunter commented 9 years ago

Updated with the required directory structure.

networkx/
     __init__.py
    addons/
        __init.py
        metis/
            metis.py
            _types.py
            enums.py
            tests/
                test_metis.py
chebee7i commented 9 years ago

Is it still necessary to have it named _types.py? Absolute imports are the default and in 2.7 you should be using

from __future__ import absolute_import
OrkoHunter commented 9 years ago

@chebee7i No, we can now keep both the names as well. Do you prefer changing it back to types.py?

chebee7i commented 9 years ago

I don't know what the reason for having an underscore should be, now that there is no conflict. So it seems like it should go back to types.py.

ysitu commented 9 years ago

Please address the comments about docstrings so that we can merge this PR and unblock some others. For the most part, this should involve only duplicating some text from enums.py into types.py.

OrkoHunter commented 9 years ago

@ysitu On it.

OrkoHunter commented 9 years ago

I should rename _types back to types right?

OrkoHunter commented 9 years ago

Docstrings added.

ysitu commented 9 years ago

I should rename _types back to types right?

With absolute imports now it makes sense to rename.

ysitu commented 9 years ago

I will merge this after the last three comments are addressed.

OrkoHunter commented 9 years ago

I will merge this after the last three comments are addressed.

Shouldn't I add the https://github.com/networkx/networkx-metis/pull/14#issuecomment-115105652 about error raised in setting the value of niter in this PR?

ysitu commented 9 years ago

You can keep the new test in #14.

OrkoHunter commented 9 years ago

Indentations corrected. _types renamed back to types and edits made in metis.py

OrkoHunter commented 9 years ago

There's a line in _metis.pyx where _types is being imported. How do you think should I make that change? With a commit in #14 or a new PR?

ysitu commented 9 years ago

It is okay to piggyback that on #14.

OrkoHunter commented 9 years ago

Okay. Thanks for merging!