[Networkit] An effort for code sanity and cleanups / Miscellaneous / GraphIO formats

Maximilian Vogel maximilian.vogel at student.kit.edu
Wed Jul 29 23:23:43 CEST 2015


Hi,

in the last days, a couple of things came to my attention on which I'm 
interested in your opinion.

*// kanboard: Code review protocol*
Often times when a task is moved to the "To Review"-column in the 
kanboard, nobody feels responsible to actually review the code. 
Therefore, I'd like to suggest the following workflow/protocol:
When moving a task to the "To Review"-column, the developer should post 
a comment with the link(s) from algohub corresponding to the changesets. 
Then, a reviewer knows what exactly to review and doesn't need to search 
for the changesets. The next step would be to assign a different person 
to the task. The assignee will then either review the code and is 
supposed to post comments on algohub or look for a different person to 
do the review for whatever reasons.

*// graph formats: GDF and VNA*
For the graph formats GDF and VNA only writers exists. As far as I see 
it, both writers are neither well coded nor up to date in terms of the 
supported features in the output. Sure, they can be fixed, but for both 
formats, it's hard to find the specifications. The gephi website has 
some information though.
But the question is, if anybody even uses these formats and (solely) 
relies on them. Since a reader and writer for Gephi's GEXF format has 
been added to networkit, I think it makes more sense, to focus on a more 
thorough support of GEXF (and GraphML) and in general more widespread 
graph formats than to support a broad range of formats half-heartedly. 
Therefore, I vote to drop both writers.

*// auxiliary/Parallelism became useless/redundant [on the C++ side]*
Since we specify OpenMP as a "hard" requirement, the wrappers in 
auxiliary/Parallelism.h became rather useless, as I also noticed that 
they aren't used throughout networkit. Currently, the only "use" is the 
re-wrapping for Python in _NetworKit.pyx. But then these should be 
wrapped there with auxiliary/Parallelism dropped entirely.
A different approach can be seen on the clang-branch[1]: The currently 
used OpenMP functions are wrapped and used throughout the code base.
Pros: Compatibility for environments where OpenMP is not available. I 
know that among others one focus is on implementations utilizing 
parallelism, but I believe that this  compatibility can be handy, even 
if it's only for testing purposes. Regarding Windows support this also 
might be useful. From a programmer's perspective, it's easy to keep the 
compatibility by just using the wrappers.
Cons: The parallelism wrapper needs to be maintained; slightly different 
function names.

[1]: 
https://algohub.iti.kit.edu/parco/NetworKit/NetworKit/changeset/1d32702c8fe8f685378908039d4a31ce4169eced

*// code sanity and cleanups*
Currently, I use the clang branch to fix compiler warnings by both gcc 
and clang with -Wextra. Although most changes are trivial, they should 
be reviewed[2]. They also raise some questions regarding coding 
guidelines. Consider the following example:
G.forEdges([&](node u, node v) {
     use(u);
});
leads to an unused parameter warning for v, which can be avoided by just 
specifying: [&](node u, node)
The question is if we want this coding style just to avoid the warnings.
Another issue were assertions like this:
assert(x >= 0);
where x is of type node or index. I deleted most of them or tried to 
replace them with a more meaningful assertion where possible.
I encourage you to look at the attached compile log, which is reduced to 
the leftover warnings and check if you are responsible for the warnings 
that I didn't take care of as they would require a deeper look at them. 
Also, it would be great if someone reviews my changes in the clang 
branch[2].
My goal is to get to results on this and the Parallelism/OpenMP issue 
soon, so that the changes can be merged into Dev and the clang-branch 
finally closed.

[2]:
https://algohub.iti.kit.edu/parco/NetworKit/NetworKit/changeset/a8f7d800c3d2f8003d2528cb1e6d07858d815d63
https://algohub.iti.kit.edu/parco/NetworKit/NetworKit/changeset/6a74ec11d881c89722c5922835158117e51a153f
https://algohub.iti.kit.edu/parco/NetworKit/NetworKit/changeset/dce0d23c55c8aa18ec18141a55240f75368b1c3b


Best,
Max


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ira.uni-karlsruhe.de/mailman/private/networkit/attachments/20150729/2132d4c8/attachment.html>
-------------- next part --------------
g++ -o .buildDbg/scd/GCE.o -c -std=c++11 -Wall -c -fmessage-length=0 -fPIC -Wextra -fopenmp -O0 -g3 -DLOG_LEVEL=LOG_LEVEL_TRACE networkit/cpp/scd/GCE.cpp
networkit/cpp/scd/GCE.cpp:117:25: warning: unused parameter 'v' [-Wunused-parameter]
  auto deltaM = [&](node v, const std::set<node>& C){
                         ^
networkit/cpp/scd/GCE.cpp:117:50: warning: unused parameter 'C' [-Wunused-parameter]
  auto deltaM = [&](node v, const std::set<node>& C){
                                                  ^

g++ -o .buildDbg/viz/FruchtermanReingold.o -c -std=c++11 -Wall -c -fmessage-length=0 -fPIC -Wextra -fopenmp -O0 -g3 -DLOG_LEVEL=LOG_LEVEL_TRACE networkit/cpp/viz/FruchtermanReingold.cpp
networkit/cpp/viz/FruchtermanReingold.cpp:84:56: warning: unused parameter 'oldLayout' [-Wunused-parameter]
  auto updateStepLength([&](std::vector<Point<float> >& oldLayout,
                                                        ^
networkit/cpp/viz/FruchtermanReingold.cpp:85:32: warning: unused parameter 'newLayout' [-Wunused-parameter]
    std::vector<Point<float> >& newLayout) {
                                ^

g++ -o .buildDbg/viz/MultilevelLayouter.o -c -std=c++11 -Wall -c -fmessage-length=0 -fPIC -Wextra -fopenmp -O0 -g3 -DLOG_LEVEL=LOG_LEVEL_TRACE networkit/cpp/viz/MultilevelLayouter.cpp
networkit/cpp/viz/MultilevelLayouter.cpp:26:52: warning: unused parameter 'Gcon' [-Wunused-parameter]
 void MultilevelLayouter::prolongCoordinates(Graph& Gcon, Graph& G) {
                                                    ^
networkit/cpp/viz/MultilevelLayouter.cpp:26:65: warning: unused parameter 'G' [-Wunused-parameter]
 void MultilevelLayouter::prolongCoordinates(Graph& Gcon, Graph& G) {
                                                                 ^

g++ -o .buildDbg/coarsening/ClusteringProjector.o -c -std=c++11 -Wall -c -fmessage-length=0 -fPIC -Wextra -fopenmp -O0 -g3 -DLOG_LEVEL=LOG_LEVEL_TRACE networkit/cpp/coarsening/ClusteringProjector.cpp
networkit/cpp/coarsening/ClusteringProjector.cpp:14:57: warning: unused parameter 'Gcoarse' [-Wunused-parameter]
 Partition ClusteringProjector::projectBack(const Graph& Gcoarse, const Graph& Gfine, const std::vector<node>& fineToCoarse,
                                                         ^

g++ -o .buildDbg/community/PLM.o -c -std=c++11 -Wall -c -fmessage-length=0 -fPIC -Wextra -fopenmp -O0 -g3 -DLOG_LEVEL=LOG_LEVEL_TRACE networkit/cpp/community/PLM.cpp
networkit/cpp/community/PLM.cpp:332:37: warning: unused parameter 'Gcoarse' [-Wunused-parameter]
 Partition PLM::prolong(const Graph& Gcoarse, const Partition& zetaCoarse, const Graph& Gfine, std::vector<node> nodeToMetaNode) {
                                     ^

g++ -o .buildDbg/community/ParallelAgglomerativeClusterer.o -c -std=c++11 -Wall -c -fmessage-length=0 -fPIC -Wextra -fopenmp -O0 -g3 -DLOG_LEVEL=LOG_LEVEL_TRACE networkit/cpp/community/ParallelAgglomerativeClusterer.cpp
In file included from networkit/cpp/community/ParallelAgglomerativeClusterer.cpp:10:0:
networkit/cpp/community/../scoring/ModularityScoring.h: In instantiation of 'void NetworKit::ModularityScoring<T>::scoreEdges(int) [with T = double]':
networkit/cpp/community/ParallelAgglomerativeClusterer.cpp:42:31:   required from here
networkit/cpp/community/../scoring/ModularityScoring.h:96:54: warning: unused parameter 'attrId' [-Wunused-parameter]
 void NetworKit::ModularityScoring<T>::scoreEdges(int attrId) {
                                                      ^


More information about the NetworKit mailing list