Some fixes to README, range construction, vertex attributes, and network-splitting functions #105

bockthom · 2018-03-10T14:51:44Z

This PR contains some minor fixes to the README.md file, a bug fix in the construct.*.ranges functions (see #103), and introduces the isolate-removal option to some more network-splitting functions.

In addition, it fixes 'Issue 1' of #102, as the function add.vertex.attribute.author.role does not need project data and aggregation level any more. By that, the performance of this function is improved.

clhunsen

Good chatches, but some things need to be polished. Please see the detailed comments for indicators.

clhunsen · 2018-03-13T08:17:00Z

util-networks-covariates.R

-    nets.with.attr = split.and.add.vertex.attribute(
-        list.of.networks, project.data, name, aggregation.level, default.value,
+    ## provide a list whose elements only contain the network as no data is needed here
+    net.list <- lapply(list.of.networks, FUN=function(net) {n <- list(); n[["network"]] <- net; return(n)})


Nice fix, but please conform to the coding guidelines. 😉 I.e., = as assignment operator, spaces around = signs and curly braces. I would like it even more when you insert line breaks for the semicolons in the anonymous function.

Ah, thanks, I did not even recognize that here. I just wanted to commit the fixes I already had in mind before vacation. Will be fixed soon ;)

clhunsen · 2018-03-13T08:19:41Z

README.md

-The classes `ProjectData` and `RangeData` hold instances of  the `NetworkConf` class, just pass the object as parameter to the constructor.
-You can also update the object at any time, but as soon as you do so, all cached data of the data object are reset and have to be rebuilt.
+The class `NetworkBuilder` holds an instance of the `NetworkConf` class, just pass the object as parameter to the constructor.
+You can also update the 'NetworkConf' object at any time by calling `NetworkBuilder$update.network.conf(...)`, but as soon as you do so, all cached data of the `NetworkBuilder` object are reset and have to be rebuilt.


The first occurrence of NetworkConf has ' instead of ` for the code environment.

clhunsen · 2018-03-13T08:20:36Z

util-misc.R

+            ranges.approx = c(start.date, end.date)
+        } else {
+            ranges.approx = c(ranges.approx, end.date)
+        }


Can you please add the change to the documentation of the function? And as mentioned in your issue (#103), please add a test for that.

Out of curiosity: When does this occur in your scripts?!

Can you please add the change to the documentation of the function? And as mentioned in your issue (#103), please add a test for that.

Sure, as already stated above, I just wanted to commit all my changes before vacation and just opened this pull request to collect those commits and give you the opportunity to already review them ;) As already provided in the issue, I will add a test for that.

Out of curiosity: When does this occur in your scripts?!

In the core-peripheral communication analysis scripts, we split already split 3-month-networks into 1-week-networks. When starting to replace Sofie's functions by the meanwhile implemented functions of the network library, I checked whether the construct.*.ranges functions also work in the described corner case. So actually, it does not occur in the scripts, but it definitely could occur some time (just think about splitting into 2-years ranges when only 1.5-year amount of data is available...)

I will add a test for that.

Just for your interest:
It was definitely worth to add a test here - by implementing a test for the described corner case, I recognized that the previously provided fix does not work in cases where overlap is not 0.
I have already fixed that, see the new commit 975ae4d soon.

Haha, nice... 😄 But that's the reason why we have a test suite. 😉

clhunsen · 2018-03-13T11:52:58Z

Ah, forgot about that: Please add the bugfixes with their respective issue numbers to the changelog.

As of the introduction of the 'NetworkBuilder' class in version v3.0, the 'NetworkConf' object is not any more held by the 'ProjectData' objects, but by the 'NetworkBuilder' object. According to that, the README.md file is updated now. Signed-off-by: Thomas Bock <[email protected]>

This is a follow-up commit for d4b8ff9, which introduced the 'remove.isolates' parameter already to the function 'split.network.time.based'. Now, we also add this option to the functions 'split.networks.time.based' and also to 'split.network.time.based.by.ranges'. Signed-off-by: Thomas Bock <[email protected]>

When adding already exisiting author-role classifications using the 'add.vertex.attribute.author.role' function, we do not need the project data, and also no aggregation level is used. Hence, the corresponding parameters are removed now. As no data has to be split any more, this also increases the performance of this function drastically. This addresses #102 and fixes Issue 1 therein. Signed-off-by: Thomas Bock <[email protected]>

When the time difference between 'start' and 'end' is smaller than 'time.period', the 'construct.*.ranges' functions produce wrong results. This commit fixes that and updates the documentation of those functions. This addresses #103. Signed-off-by: Thomas Bock <[email protected]>

Add a test for constructing ranges when the difference between start and end is smaller than the time.period. This tests the corner case addressed in commit 975ae4d. Together with 975ae4d, this fixes #103. Signed-off-by: Thomas Bock <[email protected]>

clhunsen

Looks fine now. If you do not have anything to add, you are free to merge.

clhunsen · 2018-03-20T16:55:06Z

util-networks-covariates.R

+    ## provide a list whose elements only contain the network as no data is needed here
+    net.list = lapply(list.of.networks, function(net) {
+        n = list()
+        n[["network"]] = net


Just as side note: We could have used n = list(network = net) here. But that's nothing too important. 😉

bockthom · 2018-03-20T17:03:49Z

Looks fine now. If you do not have anything to add, you are free to merge.

I will add some commits that are somehow related to the issues addressed in this PR, soon. I suggest to merge this PR after I have added those commits.

Add a new function 'match.arg.or.default' for argument verification which is able to take a specified default value. As the 'match.arg' function of R-base takes the first element of the choices vector as default, we introduce a new function to allow specifing another default value. If no default value is specified, the function 'match.arg.or.default' just calls the 'match.arg' function of R-base. Signed-off-by: Thomas Bock <[email protected]>

Use the newly introduced 'match.arg.or.default' function to verify the 'aggregation.level' and arguments used for adding vertex attributes und set their defaults. The new default values for 'aggergation.level' are 'range', except for the computation of 'first.occurrence' and 'first.activity' where the defaults are 'complete'. In addition, adapt the test for 'add.vertex.attribute.artifact.first.occurrence' accordingly and test for all aggregation levels there. This addresses issue 3 in #102. Add a TODO to the 'first activity' function to maybe later support getting first activty over all data.sources. Signed-off-by: Thomas Bock <[email protected]>

Adapt the tests for the vertex attributes 'artifact.editor.count' and 'artifact.change.count' to test all possible aggregation levels. Signed-off-by: Thomas Bock <[email protected]>

Signed-off-by: Thomas Bock <[email protected]>

If all the ranges given to 'split.data.time.based.by.ranges' are identical, just split the data once and clone the resulting range data objects. This addresses issue 2 in #102. Signed-off-by: Thomas Bock <[email protected]>

Update README.md and CONTRIBUTING.md regarding branching and tagging: From now on, we only provide fixes-branches containing backported bugfixes for the last minor version of the second last major version. We will do this only in some special cases. All the other bugfixes conerning the current major version will end up in a new bugfix version related to the current minor version. Signed-off-by: Thomas Bock <[email protected]>

Signed-off-by: Thomas Bock <[email protected]>

clhunsen

Thank you very much for the changes. There just some tiny things to polish before merging.

clhunsen · 2018-03-21T20:53:51Z

tests/test-network-covariates.R

@@ -493,14 +493,33 @@ test_that("Test add.vertex.attribute.artifact.editor.count", {

    expected.attributes = network.covariates.test.build.expected(list(1L), list(1L), list(3L, 1L))

+    expected.attributes = list(


Why don't you remove the old value in Line 494?

Ah, I missed that. Thanks for spotting. Will be removed soon.

clhunsen · 2018-03-22T08:31:52Z

util-misc.R

+match.arg.or.default = function(arg, choices, default = NULL, several.ok = FALSE) {
+
+    ## retrieve possible choices from the parent function
+    ## (the following if-block is taken from https://svn.r-project.org/R/trunk/src/library/base/R/match.R,


We should insert the permalink here instead since the trunk may change: https://svn.r-project.org/R/tags/R-3-4-4/src/library/base/R/match.R

clhunsen · 2018-03-22T08:33:52Z

util-misc.R

+    ## retrieve possible choices from the parent function
+    ## (the following if-block is taken from https://svn.r-project.org/R/trunk/src/library/base/R/match.R,
+    ##  which is also licensed under GPLv2 (or later))
+    if (missing(choices)) {


Can you please insert a short comment on the functionality in this code block since it may get complicated to comprehend the idea later. Something like the following may suffice: "If no choices are given, extract them from the formal signature of the calling function".

Sounds good. However, there already is a comment above the license statement:

## retrieve possible choices from the parent function

Should I keep that additionally, remove that, or just replace it?

Overlooked that one... 🤦‍♂️ Just enhance it. 😉

clhunsen · 2018-03-22T08:34:58Z

util-misc.R

+    }
+
+    ## check whether to return the default value
+    if (length(arg) != 1 && !several.ok && !is.null(default)){


There is a space missing before the left curly brace.

clhunsen · 2018-03-22T08:42:52Z

util-split.R

+
+        ## clone range data objects (as all ranges are identical)
+        data.split = lapply(ranges, function(x) range.data$clone())
+


Please remove the empty line here as it does not really enhance readability and you have not inserted empty lines consistently in the if-else-block. 😉

Introduce some minor fixes for the changes in PR #105: rewording, spaces, empty lines, outdated lines. Signed-off-by: Thomas Bock <[email protected]>

bockthom requested a review from clhunsen March 10, 2018 14:52

bockthom added bug enhancement labels Mar 10, 2018

bockthom added this to the v3.2 milestone Mar 10, 2018

bockthom changed the title ~~Some fixes to README, range construction, and network-splitting functions~~ Some fixes to README, range construction, author-role attribute, and network-splitting functions Mar 10, 2018

clhunsen requested changes Mar 13, 2018

View reviewed changes

bockthom added 5 commits March 19, 2018 11:03

clhunsen previously approved these changes Mar 20, 2018

View reviewed changes

bockthom added 7 commits March 21, 2018 13:17

Extend tests for some artifact vertex attributes

3c398c7

Adapt the tests for the vertex attributes 'artifact.editor.count' and 'artifact.change.count' to test all possible aggregation levels. Signed-off-by: Thomas Bock <[email protected]>

Add test for 'match.arg.or.default' function

803eef0

Signed-off-by: Thomas Bock <[email protected]>

Fix performance of 'split.data.time.based.by.ranges' function

7b5dd71

If all the ranges given to 'split.data.time.based.by.ranges' are identical, just split the data once and clone the resulting range data objects. This addresses issue 2 in #102. Signed-off-by: Thomas Bock <[email protected]>

Update changelog

5652c0b

Signed-off-by: Thomas Bock <[email protected]>

bockthom dismissed clhunsen’s stale review via 5652c0b March 21, 2018 18:26

bockthom changed the title ~~Some fixes to README, range construction, author-role attribute, and network-splitting functions~~ Some fixes to README, range construction, vertex attributes, and network-splitting functions Mar 21, 2018

clhunsen requested changes Mar 22, 2018

View reviewed changes

Minor fixes from review in PR #105

d1e15ff

Introduce some minor fixes for the changes in PR #105: rewording, spaces, empty lines, outdated lines. Signed-off-by: Thomas Bock <[email protected]>

clhunsen approved these changes Mar 22, 2018

View reviewed changes

clhunsen merged commit 7738e82 into se-sic:dev Mar 22, 2018

clhunsen mentioned this pull request Mar 22, 2018

Version 3.1.1 #108

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some fixes to README, range construction, vertex attributes, and network-splitting functions #105

Some fixes to README, range construction, vertex attributes, and network-splitting functions #105

bockthom commented Mar 10, 2018 •

edited

Loading

clhunsen left a comment

clhunsen Mar 13, 2018

bockthom Mar 19, 2018

clhunsen Mar 13, 2018

clhunsen Mar 13, 2018

bockthom Mar 19, 2018

bockthom Mar 19, 2018 •

edited

Loading

clhunsen Mar 19, 2018

clhunsen commented Mar 13, 2018

clhunsen left a comment

clhunsen Mar 20, 2018

bockthom commented Mar 20, 2018

clhunsen left a comment

clhunsen Mar 21, 2018

bockthom Mar 22, 2018

clhunsen Mar 22, 2018

clhunsen Mar 22, 2018

bockthom Mar 22, 2018 •

edited

Loading

clhunsen Mar 22, 2018

clhunsen Mar 22, 2018

clhunsen Mar 22, 2018

		@@ -493,14 +493,33 @@ test_that("Test add.vertex.attribute.artifact.editor.count", {

		expected.attributes = network.covariates.test.build.expected(list(1L), list(1L), list(3L, 1L))

		expected.attributes = list(


		## clone range data objects (as all ranges are identical)
		data.split = lapply(ranges, function(x) range.data$clone())

Some fixes to README, range construction, vertex attributes, and network-splitting functions #105

Some fixes to README, range construction, vertex attributes, and network-splitting functions #105

Conversation

bockthom commented Mar 10, 2018 • edited Loading

clhunsen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bockthom Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clhunsen commented Mar 13, 2018

clhunsen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bockthom commented Mar 20, 2018

clhunsen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bockthom Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bockthom commented Mar 10, 2018 •

edited

Loading

bockthom Mar 19, 2018 •

edited

Loading

bockthom Mar 22, 2018 •

edited

Loading