Skip to content
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

Merged
merged 27 commits into from
Apr 30, 2018

Conversation

ecklbarb
Copy link
Contributor

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.

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]>
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]>
@clhunsen
Copy link
Collaborator

The errors in the test suite come from two places in your implementation:

In both places, ARTIFACT is not defined. You would like to use private$proj.data$get.project.conf.entry("artifact") as value instead.

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]>
Copy link
Collaborator

@bockthom bockthom left a 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(
Copy link
Collaborator

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(
Copy link
Collaborator

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
Copy link
Collaborator

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?

Copy link
Contributor Author

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
#'
Copy link
Collaborator

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)) {
Copy link
Collaborator

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

Copy link
Collaborator

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"
Copy link
Collaborator

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...

Copy link
Collaborator

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:

  1. 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.
  2. When constructing artifact networks, the column artifact.type is copied to the vertex attribute kind (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`]
Copy link
Collaborator

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!

Copy link
Collaborator

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
Copy link
Collaborator

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 ..."

authors = c("Björn", "Olaf", "Karl", "Thomas")
authors = data.frame(name= c("Björn", "Olaf", "Karl", "Thomas"),
kind= TYPE.AUTHOR,
type = TYPE.AUTHOR)
Copy link
Collaborator

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")
Copy link
Collaborator

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?

Copy link
Collaborator

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.

Copy link
Collaborator

@clhunsen clhunsen left a 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
Copy link
Collaborator

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.

vertices = c("Base_Feature", "foo", "A")
vertices = data.frame(name = c("Base_Feature", "foo", "A"),
artifact.type = "feature",
kind = "feature",
Copy link
Collaborator

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?

Copy link
Collaborator

@bockthom bockthom Apr 26, 2018

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.

Copy link
Collaborator

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`]
Copy link
Collaborator

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.

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
Copy link
Collaborator

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.

@@ -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)))
Copy link
Collaborator

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,
Copy link
Collaborator

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.
Copy link
Collaborator

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():
Copy link
Collaborator

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)
Copy link
Collaborator

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"
Copy link
Collaborator

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:

  1. 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.
  2. When constructing artifact networks, the column artifact.type is copied to the vertex attribute kind (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]>
clhunsen and others added 7 commits April 27, 2018 10:33
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]>
clhunsen
clhunsen previously approved these changes Apr 27, 2018
README.md Outdated

- Mandatory edge attributes
* *`"type"`*: [`Unipartite`, `Bipartite`]
* *`"artifact.type"`*: [`"Author"`,`"File"`, `"Feature"`, `"Function"`, `"Mail"`, `"Issue"`,`"FeatureExpression"`]
Copy link
Collaborator

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
Copy link
Collaborator

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".

@clhunsen
Copy link
Collaborator

I will work on the two new comments tomorrow and fix some other issues Thomas pointed at.

Copy link
Collaborator

@bockthom bockthom left a 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) {
Copy link
Collaborator

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
Copy link
Collaborator

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) {
Copy link
Collaborator

@bockthom bockthom Apr 28, 2018

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)

Copy link
Collaborator

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

Copy link
Contributor Author

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.

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
Copy link
Collaborator

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.

ecklbarb and others added 3 commits April 30, 2018 14:14
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]>
@clhunsen
Copy link
Collaborator

@bockthom, do you request any more changes?

@bockthom
Copy link
Collaborator

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.

@bockthom bockthom merged commit 2241817 into se-sic:dev Apr 30, 2018
@clhunsen
Copy link
Collaborator

Great! Thank you very much, @ecklbarb!

@bockthom
Copy link
Collaborator

bockthom commented May 4, 2018

We have overlooked one minor thing: We have added the viridis package to the README file, but not to the install.R. I have already fixed this, please see my patch:
0001-Add-viridis-package-also-to-install.R.patch.txt

I will not open a PR just for this single commit, but in one of the next pull requests we should apply this patch...

@clhunsen clhunsen added this to the v3.2 milestone May 17, 2018
@clhunsen clhunsen mentioned this pull request May 17, 2018
fehnkera pushed a commit to fehnkera/coronet that referenced this pull request Sep 23, 2020
Adding some more comments. Include a new section in 'README.md' for
attributes.

Signed-off-by: Barbara Eckl <[email protected]>
fehnkera pushed a commit to fehnkera/coronet that referenced this pull request Sep 23, 2020
fehnkera pushed a commit to fehnkera/coronet that referenced this pull request Sep 23, 2020
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]>
fehnkera pushed a commit to fehnkera/coronet that referenced this pull request Sep 23, 2020
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]>
fehnkera pushed a commit to fehnkera/coronet that referenced this pull request Sep 23, 2020
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants