Various minor and major fixes and adjustments #111

clhunsen · 2018-03-23T16:25:05Z

Changed/Improved

Vertex and edge types (attribute "type") are now a character string (e.g., "Developer" or "unipartite") (Change/Improve values of vertex-type and edge-type constants #110, 3ca6ed9)
Default plotting layout is now igraph::layout.kamada.kawai (Segfaults when using the 'plot.network' function #109, 9099654)
Remove parameter 'color.attr' from 'motifs.search.in.network' (d33f686)
Fix and clean-up of both the plotting and the motif modules (3ca6ed9, consequence of Change/Improve values of vertex-type and edge-type constants #110)

Fixed

Probably fixed segfaults during plotting by changing the default layout (see above and issue Segfaults when using the 'plot.network' function #109)
Fix gray-scale plotting of networks (730cc54)

@ecklbarb

Due to a copy-paste error, gray-scale plotting misbehaved. This is fixed by setting proper names on the correct vectors. Thanks to @ecklbarb for pointing this out. Signed-off-by: Claus Hunsen <[email protected]>

bockthom

Nice work! However, I found some issues regarding some documentations...

In addition, please don't forget to update your copyright header in util-plot.R.
(Btw: You've also missed to do that in your latest changes to the codeface-extraction project...)

bockthom · 2018-03-24T16:55:05Z

util-plot.R

@@ -83,6 +83,9 @@ plot.print.network = function(network, labels = TRUE, grayscale = FALSE) {

 #' Construct a ggplot2/ggraph plot object for the given network.
 #'
+#' As a layout, by default, \code{igraph::layout.kamada.kawai} is used, unless a graph attribute "layout"


Could you please add some more information on how to set the layout?
E.g., add a link to the R documentation where the different layouts are listed. Such kind of information is essential to be able to use another layout.

(Just a short side note: It is pretty hard to google for the layout (e.g., layout.kamada.kawai) as the igraph api regarding layouts has changed from igraph version to igraph version. You can find a lot of outdated information and lists of the new functions layout_, but this is a little bit confusing... Is there a way to provide a link to all currently available layouts - not only the outdated ones, but also the new ones starting with layout_?)

In addition, it might be useful to add some information regarding layout (e.g., a reference to this function) also to the plot.network resp. plot.print.network functions...

Could you please add some more information on how to set the layout?
E.g., add a link to the R documentation where the different layouts are listed. Such kind of information is essential to be able to use another layout.

Will do that.

In addition, it might be useful to add some information regarding layout (e.g., a reference to this function) also to the plot.network resp. plot.print.network functions...

Will do that via copy-paste.

(Just a short side note: It is pretty hard to google for the layout (e.g., layout.kamada.kawai) as the igraph api regarding layouts has changed from igraph version to igraph version. You can find a lot of outdated information and lists of the new functions layout_, but this is a little bit confusing... Is there a way to provide a link to all currently available layouts - not only the outdated ones, but also the new ones starting with layout_?)

The function names with dots are the "old" ones, unfortunately, although those are the more readable ones. But, they are still available and aliases of the new ones. And we are already using the "old" functions in almost all places as they are more readable and contain less abbreviations (e.g., get.vertex.attribute vs. vertex_attr).
By the way, the new version of layout.kamada.kawai is layout_with_kk – which is not comprehensible rightaway.

bockthom · 2018-03-24T18:42:59Z

util-plot.R

@@ -217,24 +212,20 @@ plot.get.plot.for.network = function(network, labels = TRUE, grayscale = FALSE)
 #' @return the old network with the new and changed vertex and edge attributes
 plot.fix.type.attributes = function(network, colors.vertex = PLOT.COLORS.BY.TYPE.VERTEX, colors.edge = PLOT.COLORS.BY.TYPE.EDGE) {


Please also update the documentation of the plot.fix.type.attributes function: The comments regarding vertex.type, edge.type, vertex.type.char, edge.type.char are outdated.

Thanks for spotting that.

bockthom · 2018-03-24T18:58:08Z

util-networks.R

-TYPE.AUTHOR = 1
-TYPE.ARTIFACT = 2
+## vertex types
+TYPE.AUTHOR = "Developer"


Just one question: Does this violate our coding conventions? In our contribution guide, we state the following coding convention:
https://github.com/se-passau/codeface-extraction-r/blob/34b9d35617e0d043d86a457b56d2ebc0a5ef02cd/CONTRIBUTING.md#L182

So, what about that here?

(On a related note: To enforce our coding conventions, we should replace "developers" in the modules util-networks.R and util-core-peripheral.R, as there are three occurrences of "developer" altogether in some comments...)

Yes, it does. And I did it on purpose. 😄 The thing is that the plots are basically supposed to be used in papers – and you would rather use "developer" instead of "author" there, I guess.

But I see your point. We may want to use "Author/Developer" for the attribute. What do you think about that?
Or we add an exception to the rule just for the plotting module...

I will fix the other occurrences. Thanks for finding those.

Yes, it does. And I did it on purpose. smile The thing is that the plots are basically supposed to be used in papers – and you would rather use "developer" instead of "author" there, I guess.

I complete agree. I have already guessed that something like that was your reason using "developer" here.

But I see your point. We may want to use "Author/Developer" for the attribute. What do you think about that?

I don't like to have two names and slashes within a vertex-type name.

Or we add an exception to the rule just for the plotting module...

Hm, that's not easy to decide. As TYPE.AUTHOR is not only used in the plotting module, but also elsewhere, I would like to name it "author". (Nonetheless, when correctly incorportating committer data in #84, we somehow have to differentiate between "author" in general and "commit author" in particular to avoid too much confusion...)
To keep "developer" for plots, we then have to add additional constants in the plotting module, which define a printable type name to be used in plots. In this case, we may add confusion to the plotting module, but we have a unique naming everywhere else... and it would be easier to change the names for plotting, if required. Would that be a proper solution?

Would that be a proper solution?

Yes, it would be indeed. I will add appropriate constants to the global plot options then.

@hechtlC

When plotting networks using the default layout of the package 'ggraph' (i.e., 'igraph::layout_nicely'), we experienced segfaults in R. Therefore, we change the default layout to 'igraph::layout.kamada.kawai'. This essentially fixes #109. Thanks to @hechtlC for experiencing the problems and @bockthom for helping to find a solution. Signed-off-by: Claus Hunsen <[email protected]>

As we always match motifs on the vertex attribute 'type', the existing parameter to the function 'motifs.search.in.network' is removed. The test suite is adapted accordingly. Additionally, the documentation is updated. Signed-off-by: Claus Hunsen <[email protected]>

@bockthom

The vertex and edge types as defined in the networks module ('TYPE.AUTHOR', 'TYPE.ARTIFACT', 'TYPE.EDGES.INTRA', and 'TYPE.EDGES.INTER') are defined as magic numbers (historically coming from vector indices). We refactor them to be of type 'character'. This way, we are able to actually "read" the vertex and edge attributes without any lookup from the network objects we construct. The constants are now defined as follows: TYPE.AUTHOR = "Author" TYPE.ARTIFACT = "Artifact" TYPE.EDGES.INTRA = "Unipartite" TYPE.EDGES.INTER = "Bipartite" As the motif identification works on numerics for the vertex encoding (see 'igraph::subgraph_isomorphisms', method 'vf2'), the corresponding code is adapted to transform the (now) character constants reliably back to numerics. For this, we introduce the mapping 'MOTIF.TYPE.MAPPING' and the function 'get.vertex.types.as.numeric'. As we manually translate vertex and edge types to strings for display in the plotting module, the module gets appropriate fixes and clean-ups. Here, the legend name for author vertices is not 'TYPE.AUTHOR', but "Developer". The behavior can be overwritten via setting 'PLOT.VERTEX.TYPE.AUTHOR' (and 'PLOT.VERTEX.TYPE.ARTIFACT', respectively) appropriately. This fixes #110. Thanks to @bockthom for his numerous remarks on this change. Signed-off-by: Claus Hunsen <[email protected]>

Signed-off-by: Claus Hunsen <[email protected]>

@bockthom

In some places, we still use the word "developer" in our codebase, although we state in the contribution guide that we only use "author". This is fixed by replacing the occurrences properly. Thanks to @bockthom for pointing this out. Signed-off-by: Claus Hunsen <[email protected]>

Signed-off-by: Claus Hunsen <[email protected]>

bockthom

Now everything looks fine! Thanks again!

bockthom · 2018-03-26T14:46:30Z

Right before merging I recognized that the pull request went amongst the master branch, which is not allowed. Please open pull requests only against the dev branch!
I have already changed the base branch to dev and will merge this PR now.

Fix gray-scale plotting of networks

730cc54

Due to a copy-paste error, gray-scale plotting misbehaved. This is fixed by setting proper names on the correct vectors. Thanks to @ecklbarb for pointing this out. Signed-off-by: Claus Hunsen <[email protected]>

clhunsen added bug enhancement labels Mar 23, 2018

clhunsen added this to the 3.1.2 milestone Mar 23, 2018

clhunsen requested a review from bockthom March 23, 2018 16:25

bockthom requested changes Mar 24, 2018

View reviewed changes

clhunsen added 6 commits March 26, 2018 14:05

Update changelog

56b1f1f

Signed-off-by: Claus Hunsen <[email protected]>

Fix plot layout for sample network in showcase file

e1a339d

Signed-off-by: Claus Hunsen <[email protected]>

bockthom changed the base branch from master to dev March 26, 2018 14:41

bockthom approved these changes Mar 26, 2018

View reviewed changes

bockthom merged commit f07ee11 into se-sic:dev Mar 26, 2018

This was referenced Apr 6, 2018

Change/Improve values of vertex-type and edge-type constants #110

Closed

Segfaults when using the 'plot.network' function #109

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various minor and major fixes and adjustments #111

Various minor and major fixes and adjustments #111

clhunsen commented Mar 23, 2018

bockthom left a comment

bockthom Mar 24, 2018

clhunsen Mar 26, 2018

bockthom Mar 24, 2018

clhunsen Mar 26, 2018

bockthom Mar 24, 2018 •

edited

Loading

clhunsen Mar 26, 2018

bockthom Mar 26, 2018

clhunsen Mar 26, 2018

bockthom left a comment

bockthom commented Mar 26, 2018 •

edited

Loading

		@@ -217,24 +212,20 @@ plot.get.plot.for.network = function(network, labels = TRUE, grayscale = FALSE)
		#' @return the old network with the new and changed vertex and edge attributes
		plot.fix.type.attributes = function(network, colors.vertex = PLOT.COLORS.BY.TYPE.VERTEX, colors.edge = PLOT.COLORS.BY.TYPE.EDGE) {

Various minor and major fixes and adjustments #111

Various minor and major fixes and adjustments #111

Conversation

clhunsen commented Mar 23, 2018

Changed/Improved

Fixed

bockthom left a comment

Choose a reason for hiding this comment

bockthom Mar 24, 2018

Choose a reason for hiding this comment

clhunsen Mar 26, 2018

Choose a reason for hiding this comment

bockthom Mar 24, 2018

Choose a reason for hiding this comment

clhunsen Mar 26, 2018

Choose a reason for hiding this comment

bockthom Mar 24, 2018 • edited Loading

Choose a reason for hiding this comment

clhunsen Mar 26, 2018

Choose a reason for hiding this comment

bockthom Mar 26, 2018

Choose a reason for hiding this comment

clhunsen Mar 26, 2018

Choose a reason for hiding this comment

bockthom left a comment

Choose a reason for hiding this comment

bockthom commented Mar 26, 2018 • edited Loading

bockthom Mar 24, 2018 •

edited

Loading

bockthom commented Mar 26, 2018 •

edited

Loading