search

jacquesfize / GMatch4py

A graph matching library for Python
MIT License
193 stars 40 forks source link

Some constructive feed #33

Open NicolasMICAUX opened 2 years ago

NicolasMICAUX commented 2 years ago

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'

2) 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)

3) 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 commented 2 years ago

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 commented 2 years ago

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