franktakes / teexgraph

C++ library for large-scale network analysis and computation
GNU General Public License v3.0
24 stars 8 forks source link

Error in Building #2

Closed TennisGazelle closed 6 years ago

TennisGazelle commented 6 years ago

Running make from the top level directory, the following errors arise:

g++ -Iinclude -pedantic -Wall -march=native -fopenmp  -O3 -DNDEBUG  -o src/BDGraph.o -c src/BDGraph.cpp
In file included from src/Graph.h:52:0,
                 from src/BDGraph.cpp:17:
src/GraphTemplated.hpp: In member function ‘void Graph::printDistri(std::vector<_Tp>&, Scope)’:
src/GraphTemplated.hpp:12:28: error: there are no arguments to ‘inScope’ that depend on a template parameter, so a declaration of ‘inScope’ must be available [-fpermissive]
         if(inScope(i, scope)) {
                            ^
src/GraphTemplated.hpp:12:28: note: (if you use ‘-fpermissive’, G++ will accept your code, but allowing the use of an undeclared name is deprecated)
src/GraphTemplated.hpp: In member function ‘void Graph::printPythonNodeList(std::vector<_Tp>&, Scope, std::__cxx11::string)’:
src/GraphTemplated.hpp:114:28: error: there are no arguments to ‘inScope’ that depend on a template parameter, so a declaration of ‘inScope’ must be available [-fpermissive]
         if(inScope(i, scope)) {
                            ^
src/GraphTemplated.hpp:121:28: error: there are no arguments to ‘inScope’ that depend on a template parameter, so a declaration of ‘inScope’ must be available [-fpermissive]
         if(inScope(i, scope)) {

I'd like to assume that this is a local issue, but the beginning of the file src/GraphTemplated.hpp uses that inScope(...) function, the declaration of which I do not see before this function. Are there specific building instructions I need to run?

franktakes commented 6 years ago

Sorry for this inconvenience. I did some re-organization in the Graph.h file (see https://github.com/franktakes/teexgraph/commit/277b7f0d017202874d030e017bba9dc9c8b946b4#diff-27c57422d8ba85a574ba73f4caff1d70, which although executed a while ago was only pushed a few days ago), and apparently deleted the header for inScope(). Likely it remained unnoticed because I didn't ever make clean.

It's fixed now. Thank's for letting me know!

TennisGazelle commented 6 years ago

So after using this library (which is very fast btw; good job :) ), I was wondering if you'd be against attempts to make this conform to some OOP standards? It's potentially highly versatile and wanted as a standalone graph library for C++ that has no dependencies (other than simple data structures found in STL). In my quest to find C++ specific graph libraries that do not rely on the Boost library (for compatibility with weird OSes that dislike Boost), the only passable library that I found was yours.

Interested?

franktakes commented 6 years ago

Thanks for your positive feedback :) This sounds interesting. As you may have noticed from the code, I am not really a software engineer. But I am open to making the code more easily reusable. Roughly what kind of steps would this migration involve, and do you have experience with migrating slightly messy projects like these? ;) Any comments or pointers welcome.

TennisGazelle commented 6 years ago

Honestly, I haven't had to do major refactoring like this before, (which I kind of why I would like to contribute to an open-source project like this and get experience doing so). But below are the steps that I anticipate are necessary for restructuring the architecture and the code flow the project. Please keep in mind that I'm simply a computer science grad student with a fair amount of industry internship experience in software engineering and developer operations (dev-ops); so take everything I'm proposing in this potential makeover with a grain of salt.

To keep track of changes

The changes themselves

From the usage that I've gotten out of it, I made a few code changes to your library, only adding a little functionality, but letting the rest of your structure do the work, for the sake of your codebase's integrity. Therefore, I haven't really understood what changes are solicited yet until I understand the original design pattern and standardize another design pattern. There are a few high-level changes (epics) that I believe may be introduced right off the bat, however.

franktakes commented 6 years ago

@TennisGazelle Daniel, sorry for not getting back to you earlier; I really appreciate you summing up the very concrete steps that could be taken to move towards a standalone reusable library. It is not out of disinterest that I did not respond earlier, but for time reasons. It's great to see that you are so committed to suggesting solutions making the code of teexGraph more reusable, and it seems clear that you have some systematic experience in this area.

At the same time, I made a few assumptions about this codebase myself when I started it. First, it is primarily intended for me as a way to make my own algorithms as well algorithms from colleagues that I happen to implement, available for the community. Second, the focus is on simplicity because it keeps to code easy to read and easy to adjust, with simple data structures without any "feature envy" which may result in less performance.

That all being said, here are some responses regarding your four suggested directions of changes:

I would propose we somehow divide the work between large overhauls and quick wins. Any concrete quick wins you have in mind that we can do on the short term?

Happy to hear your thoughts :)