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.

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

Some constructive feed #33

Open
NicolasMICAUX opened this issue Nov 21, 2022 · 2 comments
Open

Some constructive feed #33

NicolasMICAUX opened this issue Nov 21, 2022 · 2 comments

Comments

@NicolasMICAUX
Copy link

NicolasMICAUX commented Nov 21, 2022

Hi! incredible promises in your Readme.md (speed through Cython, easy API, etc.)
But I found the library a bit unfinished, so here are some (hopefully) constructive feedback:

  1. Import error. After installing via git clone etc. in clean env, as required by readme.md, and just doing import gmatch4py as gm, I still encounter an import issue:
    from .embedding.deepwalk import *
  File "gmatch4py/embedding/deepwalk.pyx", line 29, in init gmatch4py.embedding.deepwalk
ModuleNotFoundError: No module named 'graph'
  1. Autocompletion not working: maybe it's just in my IDE (Pycharm Pro), but I have no autocomplete of functions signatures, or no doctsrings displayed°. Do you have on your side?
    2 bis) I know it takes time, but docs are not ultra clear for some functions (for example, how does .set_attr_graph_used works, how does it actually "use" attributes)

  2. aka 2 bis²) : why does .set_attr_graph_used always took 2 arguments, can't we just use edge attr or node attr ? It seems not, but it's not clear to me why.
    TypeError: set_attr_graph_used() takes exactly 2 positional arguments (1 given)


° I think it might be because of compiled files .so. From my researchs, it seems docstrings are dropped by defaullt. But here, they give an option to keep docstrings:
Cython.Compiler.Options.docstrings = True

Have a nice day, I hope my feedback will be useful. Again, thanks for open-sourcing your work!
Nicolas MICAUX

@jacquesfize
Copy link
Owner

Hi!

Thanks for your feedback !

Concerning the graph module import bug. Are you still in the Gmatch4py directory ? In this case, python will import unbuilt modules from the clone Gmatch4Py directory ... Maybe I should change the installation command by this one :

pip install git+https://github.com/Jacobe2169/GMatch4py.git

Regarding the auto-completion, I did not looked into it when I was developing the library. I'll try to add the Cython.Compiler.Options.docstrings = True in the setup.py.

As for attributes, I enabled the storage of nodes' attributes in my graph data structure for safety but none of the algorithms use it...

Bests,
Jacques

@NicolasMICAUX
Copy link
Author

Are you still in the Gmatch4py directory ?
No, not when I run the import script.

I enabled the storage of nodes' attributes in my graph data structure for safety but none of the algorithms use it.
ok :) Maybe you should remove the In this latest version, we add the possibility to exploit graph attributes ! from readme.md, or explain that this are not used by builtin algorithms so far

Thx for taking time to answer me, have a nice day
Nicolas

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants