-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add multi-edges to the author and artifact networks #115
Conversation
43ea94f
to
9d0edbc
Compare
Now, it is possible to construct networks which include more than one artifact and author relation. To provide this functionality, the allowed number of the 'author.relation' and the 'artifact.relation' is changed to Inf instead of 1. A further edge attribute 'relation' is added to describe the relation type. The functions 'get.artifact.network' and 'get.author.network' are changed to handle more than one relation. Therefore, the functions 'get.bipartite.network' and 'get.multi.network' are adjusted, too. The functions use the 'add.edges.for.bipartite.relations' function which can handle more than one relation now. To merge the networks of all relations, we add the functions 'merge.network.data' and 'merge.networks'. The function 'construct.network.from.list' is split into to functions 'construct.edge.list.from.key.value.list' and 'construct.network.from.edge.list'. Solves the second part of #98. Signed-off-by: Barbara Eckl <[email protected]> Signed-off-by: Claus Hunsen <[email protected]>
Now, the colors of the edges depend on the edge attribute 'relation'. The colors of the edges are not any more saved in an edge attribute, because the edge attribute is never been used. Signed-off-by: Barbara Eckl <[email protected]>
To get rid of the global variables in the plot module, we now use the color palette 'viridis' to colorize vertices and edges. The setting of the 'relation' attribute is changed accordingly. Signed-off-by: Claus Hunsen <[email protected]> Signed-off-by: Barbara Eckl <[email protected]>
Now, the different kindes of artifacts (Mail, Issue, File, Function, Feature, Featureexpression, Author) can be plotted with different colors. For this functionality, a new vertex attribute 'kind' is necessary, which includes the information of the attribute 'artifact.type' or 'Author' (for author vertices). The new vertex attribute is respected as vertex color during plotting. Signed-off-by: Barbara Eckl <[email protected]> Signed-off-by: Claus Hunsen <[email protected]>
We would respect the edge attribute 'relation' while simplifying a network, therefore, the new algorithm for simplification is now the following: First, the networks are split depending on the relation type. Then, they are simplified and, in a last step, the networks are merged to one simplified network which have parallel edges if these have different relation types. Also, this works on networks without the 'relation' attribute! Signed-off-by: Barbara Eckl <[email protected]> Signed-off-by: Claus Hunsen <[email protected]>
Signed-off-by: Barbara Eckl <[email protected]>
Remove vertex attribute 'id' of artifact networks with relation 'mail' and add vertex attribute 'artifact.type' in artifact networks of relations 'issues', 'mail' and 'callgraph'. Then these changes of the vertex attributes in artifact networks are included in the test suite. Signed-off-by: Barbara Eckl <[email protected]>
Signed-off-by: Barbara Eckl <[email protected]>
Signed-off-by: Barbara Eckl <[email protected]>
Signed-off-by: Barbara Eckl <[email protected]>
The errors in the test suite come from two places in your implementation:
In both places, |
Signed-off-by: Barbara Eckl <[email protected]>
The colors and line types of edges were sometimes wrong. To delete loops from the network before plotting, solves the problem. Signed-off-by: Claus Hunsen <[email protected]> Signed-off-by: Barbara Eckl <[email protected]>
Signed-off-by: Barbara Eckl <[email protected]>
aee37f9
to
e95afd0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR looks really good! Thanks for all the implementations, I really like the newly introduced changes!
There are just a few minor issues regarding variable naming, spacing, documentation...
(The number of comments and requested changes my seem large to you, but for the huge amount of changes the number of my comments is comparably small - and it is only some minor stuff...)
If anything is unclear, don't hesitate to ask.
util-networks.R
Outdated
private$proj.data$get.thread2author(), | ||
network.conf = private$network.conf, | ||
directed = private$network.conf$get.value("author.directed") | ||
) | ||
author.relation = construct.network.from.edge.list( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this variable called author.relation
? It would be more comprehensible to use the same names as for cochange networks: author.net
(and also author.net.data
a few lines above).
util-networks.R
Outdated
private$proj.data$get.issue2author(), | ||
network.conf = private$network.conf, | ||
directed = private$network.conf$get.value("author.directed") | ||
) | ||
author.relation = construct.network.from.edge.list( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: It would be more comprehensible to use author.net
instead of author.relation
here.
util-networks.R
Outdated
mail = private$get.author.network.mail(), | ||
issue = private$get.author.network.issue(), | ||
stop(sprintf("The author relation '%s' does not exist.", rel)) | ||
## TODO get edge and vertex data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question: What does the TODO
stand for? What would you like to implement here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I plan to exchange the networks with the network data. Then it would be more efficient to merge the network data instead of the networks.
util-networks.R
Outdated
} | ||
|
||
#' Merges a list of network to one big network | ||
#' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plural s is missing here: list of networks
util-networks.R
Outdated
vertex.data = igraph::as_data_frame(network, what = "vertices") | ||
|
||
## select edges of one relation, build the network and simplify this network | ||
if ("relation" %in% colnames(edge.data)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to also check whether only one relation is used in the whole network and perform the former simplification then? I am not sure about the performance overhead here, but it may slow down the simplification when constructing the network from the data frame again...
(I did not try that - so, maybe I am completely wrong and there is not really an overhead - but it would be worth to check, as most of our current analyses use author networks with only one relation and we should not slow down our current analyses, if possible.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I totally support that.
Additionally, we can improve the code by using "relation" %in% igraph::list.vertex.attributes(network)
as condition for the if-statement. Then, we move the data extraction into the if-block.
@@ -216,6 +216,9 @@ read.mails = function(data.path) { | |||
## set pattern for thread ID for better recognition | |||
mail.data[["thread"]] = sprintf("<thread-%s>", mail.data[["thread"]]) | |||
|
|||
## set proper artifact type for proper vertex attribute 'artifact.type' | |||
mail.data["artifact.type"] = "Mail" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A general question @clhunsen: What is this artifact type used for? Wouldn't it be more comprehensible to use "Mail Thread" as one artifact is not a simple mail but a mail thread? I know that a change here would also entail a lot of other changes, but we should think about that to be more precise here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The column artifact.type
– that is now present in all data sources – is used in two places:
- It is used as a configurable edge attribute. Formerly, it was only present for commit data, but it makes sense to have this attribute consistently for all data sources and, therefore, artifact and author relations.
- When constructing artifact networks, the column
artifact.type
is copied to the vertex attributekind
(e.g., see here). This attribute is later used for plotting, for example.
We need to talk about the capitalization here, as I mentioned above. Right now, I am still a bit confused and need to debug this tomorrow.
README.md
Outdated
- *`"name"`* | ||
- *`"type"`*: [`Author`, `Artifact`] | ||
- `"artifact.type"`: [`file`, `feature`, `function`,`mail`, `issue`,`featureexpression`] | ||
- *`"kind"`*: [`author`,`file`, `feature`, `function`,`mail`, `issue`,`featureexpression`] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very important part which we should definitively add to the README file. However, this is not the correct place, I guess, since those vertex attributes are not configurable via the network configuration (which this section is about). Any ideas on that @clhunsen ?
I would suggest to introduce a new section regarding vertex attributes (###) after the NetConf section. There we also can add some statements regarding the configurable vertex attributes (see util-networks-covariates.R
) as those are still missing in the README.
In addition, you use the package viridis
now, but it is not added to the ### Needed R packages
section of the README. Please add that. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right, @bockthom. The same holds for the relation
attribute on edges, by the way (Line 242)! We definitely need to talk about the mandatory edge attributes and also the mandatory vertex attributes in the README file, but in a separate section, as they are not configurable and not exposed through the network configuration.
I would suggest to add a new section called Network properties
and to put the mandatory attributes there (type
, relation
, date
on edges; type
and kind
on vertices) in the same style as you have done here. So, no sophisticated descriptions, but just two plain lists. When there is time, I will add some more text on the existing additional vertex attributes in the future.
NEWS.md
Outdated
- edit function `get.author.network` to handle more than one relation | ||
- edit function `get.artifact.network` to handle more than one artifact relation | ||
- add loop for handle more than one relation type and merge the resulting vertex lists and edge lists in `get.bipartite.network` | ||
- add loops for different relations for `authors.to.artifacts` in `get.multi.network`, add information about the relation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we are here in a network context, loop
can be a self-edge, but here the loop in the program structure is meant. What about removing the word "loop" here?
Suggestion: "handle more than ..." and "enable different relations for ..."
tests/test-networks-author.R
Outdated
authors = c("Björn", "Olaf", "Karl", "Thomas") | ||
authors = data.frame(name= c("Björn", "Olaf", "Karl", "Thomas"), | ||
kind= TYPE.AUTHOR, | ||
type = TYPE.AUTHOR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the indentation is different than in the other changes (shift kind and type by one space).
In addition, there are spaces missing before =
at kind and name.
util-conf.R
Outdated
@@ -89,7 +90,7 @@ Conf = R6::R6Class("Conf", | |||
#' - allowed, and | |||
#' - allowed.number. | |||
check.value = function(value, name) { | |||
browser(expr = name == "revisions") | |||
# browser(expr = name == "revisions") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@clhunsen Just a small question: What was the reason for introducing those browser
statements some time ago?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the very browser()
statement that is needed to debug the updating of revision data after splitting. Not sure, why this was committed before, but I does not hurt for the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, the changes are really good. I like the new vertex and edge attributes, and also the overhauled plotting. Thank you very much for your work.
There are many small remarks from my side. Additionally, we need to discuss the consistency of the new attributes in a face-to-face meeting, I guess. Anyway, I will spend some time in the next days to debug the code regarding this (in-)consistency to get a clearer idea how it's working exactly.
If and when you need support, just tell me.
@@ -1,5 +1,47 @@ | |||
# codeface-extraction-r – Changelog | |||
|
|||
## unversioned |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bockthom: Just as a reminder: When packing the next release, we need to combine the duplicate headings named ## unversioned
.
tests/test-networks-artifact.R
Outdated
vertices = c("Base_Feature", "foo", "A") | ||
vertices = data.frame(name = c("Base_Feature", "foo", "A"), | ||
artifact.type = "feature", | ||
kind = "feature", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we handle capitalization for the various new attributes on edges and vertices? I am not sure this is handled consistently.
artifact.type
and kind
on vertices are in lower case, except for "Author"
; artifact.type
on edges seems to be in upper case (e.g., see below in Line 61), while relation
is in lower case again.
I would prefer uppercase, I guess, to get consistency to the type
-attribute values. @bockthom, what is your opinion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The important thing is to make it consistent. For me it does not matter whether to use uppercase or lowercase. In most cases we currently use lowercase, I guess. To keep existing scripts working, I would suggest to user lowercase. Otherwise we would have to adapt lots of scripts where we use relations and artifacts with lowercase. Keep in mind that we currently also have lots of functions which use the data source as parameter - and in all those cases we use lowercase characters... Should we also fix those things to stay consistent? This also entails lots of changes in former scripts...
Summary: Uppercase would be nicer but implies more effort to adapt all scripts. Both options would be fine for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for bookkeeping purposes: We agreed on using uppercase words for the vertex attributes and lowercase words for the relations (see 84516fc). Additionally, we cleared the confusion between vertex attributes and data sources, which are related, but not that close sometimes that it would matter here.
README.md
Outdated
- *`"name"`* | ||
- *`"type"`*: [`Author`, `Artifact`] | ||
- `"artifact.type"`: [`file`, `feature`, `function`,`mail`, `issue`,`featureexpression`] | ||
- *`"kind"`*: [`author`,`file`, `feature`, `function`,`mail`, `issue`,`featureexpression`] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right, @bockthom. The same holds for the relation
attribute on edges, by the way (Line 242)! We definitely need to talk about the mandatory edge attributes and also the mandatory vertex attributes in the README file, but in a separate section, as they are not configurable and not exposed through the network configuration.
I would suggest to add a new section called Network properties
and to put the mandatory attributes there (type
, relation
, date
on edges; type
and kind
on vertices) in the same style as you have done here. So, no sophisticated descriptions, but just two plain lists. When there is time, I will add some more text on the existing additional vertex attributes in the future.
tests/test-networks-multi-relation.R
Outdated
date = get.date.from.string(c("2016-07-12 15:58:59", "2016-07-12 16:00:45", "2016-07-12 16:05:41", ## author cochange | ||
"2016-07-12 16:06:10", "2016-07-12 16:05:41", "2016-07-12 16:06:32", | ||
"2016-07-12 16:06:10", "2016-07-12 16:06:32", | ||
"2016-07-12 15:58:40", "2016-07-12 15:58:50", "2016-07-12 16:04:40", ## author mail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like those comments here (such as ## author mail
) that you use to annotate the different blocks in the edge-list data. Can you move insert such comments also for all other columns of the data.frame and also the other tests? May not be needed to have the comments everywhere, but the line breaks after each block may be a good start.
I know that seems tedious, but understanding the tests and their structure may help identifying problems in the future.
By the way, we use ##
as a comment only if the comment is isolated on a line. When adding a comment after code on the same line, a single #
is sufficient. But this is not too important here.
tests/test-networks-multi.R
Outdated
@@ -83,7 +85,8 @@ test_that("Construction of the multi network for the feature artifact with autho | |||
"Base_Feature", "Base_Feature", "foo", "A", "A", "Base_Feature", "Base_Feature", "Base_Feature", | |||
"foo"), | |||
weight = c(1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1), | |||
type = c(rep(TYPE.EDGES.INTRA, 10), rep(TYPE.EDGES.INTER, 6))) | |||
type = c(rep(TYPE.EDGES.INTRA, 10), rep(TYPE.EDGES.INTER, 6)), | |||
relation =c(rep("cochange", 16))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a space missing after =
.
util-networks.R
Outdated
networks = lapply(unique(edge.data[["relation"]]), | ||
function(relation) { | ||
network.data = edge.data[edge.data[["relation"]] == relation, ] | ||
net = igraph::graph_from_data_frame(d=network.data, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment here (as we have one for the network construction):
## TODO perform simplification on edge list?
util-plot.R
Outdated
@@ -90,17 +75,16 @@ plot.network = function(network, labels = TRUE, grayscale = FALSE) { | |||
#' | |||
#' Note: The names for the vertex types are taken from the variables \code{PLOT.VERTEX.TYPE.AUTHOR} and | |||
#' \code{PLOT.VERTEX.TYPE.ARTIFACT}. The defaults are \code{"Developer"} and \code{TYPE.ARTIFACT}, respectively. | |||
#' All loops are deleted before plotting the network. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(and above) 😉
util-plot.R
Outdated
ggraph::scale_edge_linetype(name = "Relations") + | ||
ggplot2::discrete_scale(name = "Relations", "edge_colour", "viridis", | ||
viridis::viridis_pal(option = "viridis", end = 0.8, begin = 0.25)) + | ||
## BROKEN RIGHT NOW due to bug in scale_edge_colour_viridis(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future, I would like to use the code that is commented right now as it is high-level API, while the currently used one is low-level API. Unfortunately, there is a bug in the ggraph
package when using the viridis
package's API (see here). The bug is already fixed upstream, but not in the CRAN package as it seems. At least, the code did not work for me after upgrading ggraph
to the latest version..
util-plot.R
Outdated
# ggraph::scale_edge_colour_viridis(name = "Relations", option = "magma", discrete = TRUE, | ||
# end = 0.85, begin = 0, direction = 1) + | ||
|
||
# ## scale vertices (colors and styles) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess, we can remove the old code in this comment block now.
@@ -216,6 +216,9 @@ read.mails = function(data.path) { | |||
## set pattern for thread ID for better recognition | |||
mail.data[["thread"]] = sprintf("<thread-%s>", mail.data[["thread"]]) | |||
|
|||
## set proper artifact type for proper vertex attribute 'artifact.type' | |||
mail.data["artifact.type"] = "Mail" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The column artifact.type
– that is now present in all data sources – is used in two places:
- It is used as a configurable edge attribute. Formerly, it was only present for commit data, but it makes sense to have this attribute consistently for all data sources and, therefore, artifact and author relations.
- When constructing artifact networks, the column
artifact.type
is copied to the vertex attributekind
(e.g., see here). This attribute is later used for plotting, for example.
We need to talk about the capitalization here, as I mentioned above. Right now, I am still a bit confused and need to debug this tomorrow.
Adding some more comments. Include a new section in 'README.md' for attributes. Signed-off-by: Barbara Eckl <[email protected]>
Signed-off-by: Barbara Eckl <[email protected]>
Signed-off-by: Claus Hunsen <[email protected]>
In this patch, we improve the recently introduced vertex and edge attributes. Details are listed below. Changes regarding vertex attributes: - The attribute 'type' is either TYPE.AUTHOR or TYPE.ARTIFACT. - The attribute 'kind' now contains solely artifact types (such as "Feature" or "Function") and "Author" as values. - The attribute 'artifact.type' is removed completely as it has been partly redundant to the 'kind' attribute. Changes regarding edge attributes: - The attribute 'type' is either TYPE.EDGES.INTRA or TYPE.EDGES.INTER. - The attribute 'artifact.type' is sourced solely from the column 'artifact.type', present in all data sources. - The attribute 'relation' is filled by the NetworkConf values for 'author.relation' or 'artifact.relation', respectively. Additionally, some TODO items are added or adapted. Signed-off-by: Claus Hunsen <[email protected]> Signed-off-by: Barbara Eckl <[email protected]>
To reliably resolve the correct vertex attribute 'kind' for the current artifact relation, we now use a mapping method. This way, we are open to rename any 'kind' value in later steps. Signed-off-by: Claus Hunsen <[email protected]>
As the vertices that we produce in the artifact relation 'mail' represent mail threads, the corresponding vertex kind is renamed to 'MailThread'. Signed-off-by: Claus Hunsen <[email protected]>
As the edges that we produce for the relation 'issue' are initiated by an issue comment – and not the complete issue–, the corresponding artifact type is renamed to 'IssueComment'. Signed-off-by: Claus Hunsen <[email protected]>
For consistent and comprehensible legends for network plots, the legend keys for shapes/linetypes and colors are properly distinguished now. Additionally, the size of the vertex colors are reduced in size to properly match the one of the vertex shapes. Signed-off-by: Claus Hunsen <[email protected]>
Add the parameters 'remove.multiple' and 'remove.loops' to the functions 'simplify.network' and 'simplify.networks' and pass the them to the 'igraph::simplify' function. Signed-off-by: Thomas Bock <[email protected]>
As the edges that we produce for the relation 'issue' are not only initiated by an issue comment, but also by other events, for example opening or closing the issue, the corresponding artifact type is renamed to 'IssueEvent'. Signed-off-by: Barbara Eckl <[email protected]>
README.md
Outdated
|
||
- Mandatory edge attributes | ||
* *`"type"`*: [`Unipartite`, `Bipartite`] | ||
* *`"artifact.type"`*: [`"Author"`,`"File"`, `"Feature"`, `"Function"`, `"Mail"`, `"Issue"`,`"FeatureExpression"`] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to update the values here and for kind
above:
- remove artifact type "Author",
- change artifact type "Issue" to "IssueEvent", and
- change vertex kind "Mail" to "MailThread".
util-networks.R
Outdated
|
||
## set artifact.type attribute | ||
attr(bip.relation, "artifact.type") = artifact.type | ||
#' @return the list of data for the bipartite relations, with the attributes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to update the documentation here: insert "each" after the comma and change the information on "artifact.type" to "vertex.kind".
I will work on the two new comments tomorrow and fix some other issues Thomas pointed at. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look very good now. I am really looking forward to having available the new functionality in the network library.
Besides the minor improvements regarding the superfluous graph attribute I have talked about with Claus yesterday, there are some minor issues regarding the documentation (some of them even existed before this PR, but when touching the functions, we could (and IMHO should) also fix the documentation in the same breath.)
So, we are very close to merging now ;)
util-networks.R
Outdated
|
||
get.vertex.kind.for.relation = function(relation) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation for this function is missing...
util-networks.R
Outdated
#' For example for a list of authors per thread, where all authors are connected if they are | ||
#' in the same thread (sublist). | ||
#' | ||
#' Important: The input needs to be compatible with the function \code{get.key.to.value.from.df}. | ||
#' | ||
#' If directed the order of things in the sublist is respected and the 'edge.attr's hold the | ||
#' vector of possible edge attributes in the given list. | ||
#' | ||
#' @param list the list of lists with data | ||
#' @param network.conf the network configuration | ||
#' @param directed whether or not the network should be directed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor thing: Here, the default value of "directed" is missing in the documentation.
(It was missing also before this PR, but it would be nice to fix some previous minor issues when changing those functions anyway.)
util-networks.R
Outdated
#' @param directed whether or not the network should be directed | ||
#' | ||
#' @return the built network | ||
construct.network.from.edge.list = function(vertices, edge.list, network.conf, directed = TRUE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: The default value for "directed" is missing in the documentation.
Anyway, why is the default value for the directed
parameter of construct.edge.list.from.key.value.list
FALSE
, but here for construct.network.from.edge.list
TRUE
? Is there a special reason for that? Otherwise I would suggest to use the same default value for both functions (which should be FALSE
I guess, as the default value previously was FALSE
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I have updated the comment above as I have mixed the TRUE
and FALSE
values inadvertently, now the above comment should be correct.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose for both functions the default value FALSE
.
Signed-off-by: Barbara Eckl <[email protected]>
util-networks.R
Outdated
#' Determine which vertex kind should be chose for the vertex depending on the relation | ||
#' between the vertices. | ||
#' | ||
#' @return the vertex kind to be used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@param relation ...
is missing here.
Signed-off-by: Barbara Eckl <[email protected]>
As discussed regarding PR #115 and commit 4dead55, the resolution of the vertex kind for artifact networks is now solved through using the available relation variable. This enables us to remove the graph attributes we added earlier. Thanks to @bockthom for pointing this out. Signed-off-by: Claus Hunsen <[email protected]>
To enhance intercompatibility with internal functionality from the package 'igraph', especially with the function 'igraph::as.data.frame(..., what = "both")', the function 'construct.edge.list.from.key.value.list' now returns a data.frame for the vertices, not a plain vector. Signed-off-by: Claus Hunsen <[email protected]>
@bockthom, do you request any more changes? |
No, everything is fine now, I guess, as the documentation is fixed and the unnecessary graph attributes are removed. Thanks to @ecklbarb for working on this very interesting and useful new functionality - and also thanks for fixing all the minor things. As all the changes (which are quite a lot...) look good now, I will merge this PR now. |
Great! Thank you very much, @ecklbarb! |
We have overlooked one minor thing: We have added the I will not open a PR just for this single commit, but in one of the next pull requests we should apply this patch... |
Adding some more comments. Include a new section in 'README.md' for attributes. Signed-off-by: Barbara Eckl <[email protected]>
Signed-off-by: Barbara Eckl <[email protected]>
As discussed regarding PR se-sic#115 and commit 4dead55, the resolution of the vertex kind for artifact networks is now solved through using the available relation variable. This enables us to remove the graph attributes we added earlier. Thanks to @bockthom for pointing this out. Signed-off-by: Claus Hunsen <[email protected]>
As with the changes in PR se-sic#115 the 'viridis' package was added as needed package, also update the install.R. In addition, fix the misspelling of a variable name in install.R. Signed-off-by: Thomas Bock <[email protected]>
After merging PR se-sic#115, we can remove the function 'combine.networks' and rely completely on the functions 'igraph::disjoint_union' and 'add.edges.for.bipartite.relation'. This improves readability and minimizes the interface of the file 'util-networks.R'. The function 'igraph::disjoint_union' has been chosen instead of 'merge.networks' because the runtime of the chosen function is most likely lower in this specific case as the other function is far more generic. This fixes se-sic#118. Signed-off-by: Claus Hunsen <[email protected]>
I added to the implementation of the networks the functionality to build networks with multiple edge relations. To enable multi-edge networks, it is necessary to handle more than one relation type. These changes are implemented in util-networks.R, util-plot.R, util-conf.R and the relating test files.
This is one part of #98.