Improve add.vertex.attribute.first.activity #135

klaraschlueter · 2018-07-31T14:22:44Z

Addressing issue #92 , the function is now able to

compute the first activity of multiple data sources and include all of them in one vertex attribute
compute a global first activity over all given data sources

clhunsen

Thank you very much for this neat implementation for the 'first.activity' attribute, @klaraschlueter. Nice work! 👍

Except for the comments on the code, please work on the following commits:

Commit 59172de: Please remove this commit by squashing it with the next one (733fa79) – while using the second commit's commit message.
Commit 0081a62: Please remove the 's' from all verbs.
Commit 8f342c3: Remove the forward references to the upcoming commits, just use backward references. You can write: "Tests for the other two cases will be added in upcoming commits."
Commit 3f8ef22: analogously to 8f342c3.

clhunsen · 2018-07-31T15:24:13Z

util-networks-covariates.R

-#'                      One of \code{mails}, \code{commits}, and \code{issues}. [default: "mails"]
-#' @param name The attribute name to add [default: "first.activity"]
+#' @param activity.types The kinds of activity to use as basis.
+#'                      One ore more of \code{mails}, \code{commits} and \code{issues}.


The default value is missing: [default: c("mails", "commits", "issues")].

and or instead of ore

clhunsen · 2018-07-31T15:24:43Z

util-networks-covariates.R

-#' @param default.value The default value to add if a vertex has no matching value [default: NA]
+#' @param default.value The default value to add if a vertex has no matching value [default: NA].
+#' @param compute.over.all Flag indicating that one the first activity over all given
+#'                         \code{activity.types} is of interest (instead of one value per type)


[default: FALSE]

In addition, there is something wrong in the sentence structure: "Flag indicating that one the first activity..." Do you mean only instead of one ?

clhunsen · 2018-07-31T15:32:48Z

util-networks-covariates.R

+#'                                 A correctly formatted dataframe can easily be created by the function \code{compute.first.activities.dataframe}.
+#'
+#' @return A list containing for each person in the given dataframe a list containing the first activity of all activity types in the given dataframe.
+list.over.all = function(first.activity.dataframe) {


Please add some more inline documentation for this function.

clhunsen · 2018-07-31T15:32:54Z

util-networks-covariates.R

+#'                                 A correctly formatted dataframe can easily be created by the functione \code{compute.first.activities.dataframe}.
+#'
+#' @return A list containing for each person in the given datafram a list containing for each activity type in the given dataframe the first activity.
+list.single.types = function(first.activity.dataframe) {


Please add some more inline documentation for this function.

clhunsen · 2018-07-31T15:33:47Z

util-networks-covariates.R

+#' Helper function for first activity: Converts the given dataframe in a list containing the first activity per activity type and person.
+#'
+#' @param first.activity.dataframe A dataframe with the first activity data. The rows are persons, the coloums are activity type functions.
+#'                                 A correctly formatted dataframe can easily be created by the functione \code{compute.first.activities.dataframe}.


clhunsen · 2018-08-01T06:54:06Z

util-networks-covariates.R

                                               name = "first.activity",
                                               aggregation.level = c("range", "cumulative", "all.ranges",
                                                                     "project.cumulative", "project.all.ranges",
                                                                     "complete"),
-                                               default.value = NA) {
+                                               default.value = NA,
+                                               compute.over.all = FALSE) {


I would like to rename this parameter to something like single.value or compute.overall.minimum. @bockthom, what's your opinion on that?

I dislike single.value as it does not really state what it is about.
compute.overall.minimum sounds better.

I don't think talking about a minimum is appropriate here. The function encapsulates the computation of the first activity. There is no need to reveal to the user, that this is done by calculating the minimum of all his activities. If you want me to append the "what-are-we-computing", then I'd like to say "compute.overall.first.activity".

But in general, by appending something to the parameter name, I think the question we should answer is "over all what are we computing?", and not "what are we computing?", as the answer to the second question is the semantics of the whole function. Hence, I suggest "compute.overall.activity.types".

You are right, @klaraschlueter. In hindsight, "minimum" is the wrong suffix. 🤦‍♂️ Thanks for pointing this out.

I really like your first suggestion: compute.overall.first.activity. Maybe, even the abbreviated version would suffice: compute.overall.first.

Good point, @klaraschlueter. If we really want to be precise here, then we should describe what the "over all" is related to. Without explanation, "over all" could mean "over all developers" or "over all aggregation levels" or "over all ranges", ... - So, when willing to be more precise, then we should state that the "over all" refers to the activity types and nothing else. Hence, the most precise name would be compute.first.activity.over.all.activity.types. However, that is way to long and contains information which is already present in the function name.

What about something like take.first.of.all.activity.types - sounds still a little bit strange, but is not too long and contains all necessary information. Are there better ideas?

I like "take.first.of.all.activity.types"! Except, I think replacing "of" by "over" clarifies, that we are not computing a first activity of each type, but one over all. Would "take.first.over.all.activity.types" be okay?

I do not think that all information needs to be incorporated in the parameter name, that is the reason why we have documentation, right? However, overall, I am fine with @klaraschlueter's last suggestion.

clhunsen · 2018-08-01T06:58:33Z

util-networks-covariates.R

+    compute.attr = function(range, range.data, net) {
+        df = get.first.activity.data(activity.types, range.data)
+        if(compute.over.all) {
+            return(list.over.all(df))


We definitely need to rename this function and alsolist.single.typesbelow: I propose to prefix both functions with get.first.activity.data. to show the relation to the function get.first.activity.data.
@bockthom, what are your thoughts on that?

For clarification: We are talking about the function list.over.all, not the inner function here ;)

Regarding the list.single.types and list.over.all: I agree on renaming those functions by adding the proposed prefix.

@clhunsen We should think about adding an additional sentence to the README.md file. Currently we only state:

To add further vertex attributes [...] please see the file `util-networks-covariates.R` for the set of corresponding functions to call
(Btw, there are two typos in the sentence in README.md as the a in "please" is missing and there is a wrong n in "covariates". We should fix that anyway).

I suggest to add information which functions are designated to be used by the end user - there are so many helper functions now, that it is not that easy to find out which functions are designated for the end user. What about the following:

To add further vertex attributes – which can only be done after constructing a network –, please see the functions `add.vertex.attribute.*` in the file `util-networks-covariates.R` for the set of corresponding functions to call.

Do you have other ideas how to clarify which of those functions are designated for the end user?

For clarification: We are talking about the function list.over.all, not the inner function here ;)

Yeah, that's the one. 😉😄

Regarding the list.single.types and list.over.all: I agree on renaming those functions by adding the proposed prefix.

👍

@clhunsen We should think about adding an additional sentence to the README.md file. Currently we only state:

To add further vertex attributes [...] please see the file util-networks-covariates.R for the set of corresponding functions to call
(Btw, there are two typos in the sentence in README.md as the a in "please" is missing and there is a wrong n in "covariates". We should fix that anyway).

I will do that in my upcoming PR. Your suggestion is fine. 👍

clhunsen · 2018-08-01T07:05:27Z

util-networks-covariates.R

+#'
+#' @return A data frame with rows named with persons and colums named with activity types, containing the time of the corresponding
+#'         first activity as POSIXct.
+get.first.activity.data = function(activity.types = c("commits", "mails", "issues"), range.data) {


Please switch the two parameters in their order. Optional parameters should be always at the end of the parameter list.

clhunsen · 2018-08-01T07:07:56Z

util-networks-covariates.R

+
+#' Helper function for first activity: Converts the given dataframe in a list containing the first activity per activity type and person.
+#'
+#' @param first.activity.dataframe A dataframe with the first activity data. The rows are persons, the coloums are activity type functions.


clhunsen · 2018-08-01T07:08:12Z

util-networks-covariates.R

+#' @param first.activity.dataframe A dataframe with the first activity data. The rows are persons, the coloums are activity type functions.
+#'                                 A correctly formatted dataframe can easily be created by the functione \code{compute.first.activities.dataframe}.
+#'
+#' @return A list containing for each person in the given datafram a list containing for each activity type in the given dataframe the first activity.


bockthom

Thank you for improving the computation of the first activity. Looks really good - and also looks like a lot of work, more than I would have expected to fix this vertex attribute.

However, one remark regarding your documentation of the functions:

Could you please move the default value to the end of the line. That is, even after the full stop?

It would be much easier to read if the default value is not part of the sentence but an individual part of the documentation. So, for example, I would expect the following format:

#' @param param.name The description of the param. [default: value]

I am sorry for being that picky.

bockthom · 2018-08-01T09:08:30Z

util-networks-covariates.R

-            )
+    compute.attr = function(range, range.data, net) {
+        df = get.first.activity.data(activity.types, range.data)
+        if(compute.over.all) {


Here a space is missing after if

Contains only basic computation. Tests aren't adapted. The function add.vertex.attribute.first.activity is able to handle multiple data sources now. The result is stored as a list containing all minimums, named with the corresponding data source. Signed-off-by: Klara Schlueter <[email protected]>

Concerns method add.vertex.attribute.first.activity. If no information is found for some person and data source, than the corresponding element of the list added as vertex atttribute is set to NA (instead of not being created). Signed-off-by: Klara Schlueter <[email protected]>

A flag of the function add.vertex.attribute.first.activity now indicates, if the first acticity should be computed and stored per single data source or "globally" over all given data sources. Signed-off-by: Klara Schlueter <[email protected]>

Improve naming and add helper methods. Signed-off-by: Klara Schlueter <[email protected]>

One of three cases is tested: given multiple data sources, compute the first activity per person and datasource. Tests for the other two cases are added in commit will be added in upcomming commits. Signed-off-by: Klara Schlueter <[email protected]>

Tests one of three cases: compute the first activity per person over all given data sources (one value per person). Tests for the other two cases are added in commit 92241506265c5d262946d71add5ed2d680217599 and one future commit. Signed-off-by: Klara Schlueter <[email protected]>

Signed-off-by: Klara Schlueter <[email protected]>

Tests one of three cases: given one data source, compute the first activity per person per data source. Tests for the other two cases were added in commit 92241506265c5d262946d71add5ed2d680217599 and commit d7ebb79dbb4a5b2dd8102255f1ebc7bea4513029. Signed-off-by: Klara Schlueter <[email protected]>

Rename function, rectify documentation structure, delete browser statement. Signed-off-by: Klara Schlueter <[email protected]>

Signed-off-by: Klara Schlueter <[email protected]>

Outdated.

clhunsen · 2018-08-20T13:31:20Z

Thank you very much for your work, @klaraschlueter! 👍 Sorry that this took so long.

@bockthom

As pointed out in PR se-sic#135, the instructions on adding vertex attributes to constructed networks get more and more confusing due to the increasing number of helper functions in the file `util-networks-covariates.R`. To be more precise, the functions to add vertex attributes are now described with the pattern "add.vertex.attribute.*" in the README file. Additionally, some typos are fixed. Props to @bockthom for pointing this out. Signed-off-by: Claus Hunsen <[email protected]>

clhunsen added the enhancement label Jul 31, 2018

clhunsen requested changes Aug 1, 2018

View reviewed changes

clhunsen mentioned this pull request Aug 1, 2018

Further vertex attributes #92

Closed

4 tasks

bockthom previously requested changes Aug 1, 2018

View reviewed changes

klaraschlueter force-pushed the attribute-active.ranges branch from 67a9925 to 8998645 Compare August 5, 2018 07:17

Klara added 12 commits August 17, 2018 18:49

Refactor add.vertex.attribute.first.activity

334c258

Improve naming and add helper methods. Signed-off-by: Klara Schlueter <[email protected]>

Remove unnecessary type conversation from first.activity test

b0ffe62

Signed-off-by: Klara Schlueter <[email protected]>

Minor changes

24ffbbb

Rename function, rectify documentation structure, delete browser statement. Signed-off-by: Klara Schlueter <[email protected]>

Improvements suggested by reviews

ca0dfde

Signed-off-by: Klara Schlueter <[email protected]>

Fix typo in documentation

67eaae1

Signed-off-by: Klara Schlueter <[email protected]>

Update NEWS.md

8bc3236

Signed-off-by: Klara Schlueter <[email protected]>

klaraschlueter force-pushed the attribute-active.ranges branch from 1abdaee to 8bc3236 Compare August 17, 2018 16:55

clhunsen approved these changes Aug 20, 2018

View reviewed changes

clhunsen merged commit 06ed129 into se-sic:dev Aug 20, 2018

clhunsen mentioned this pull request Aug 25, 2018

Many little improvements and fixes #140

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve add.vertex.attribute.first.activity #135

Improve add.vertex.attribute.first.activity #135

klaraschlueter commented Jul 31, 2018

clhunsen left a comment

clhunsen Jul 31, 2018

bockthom Aug 1, 2018

clhunsen Jul 31, 2018

bockthom Aug 1, 2018

clhunsen Jul 31, 2018

clhunsen Jul 31, 2018

clhunsen Jul 31, 2018

clhunsen Aug 1, 2018

bockthom Aug 1, 2018

klaraschlueter Aug 2, 2018

clhunsen Aug 3, 2018

bockthom Aug 3, 2018

klaraschlueter Aug 3, 2018

clhunsen Aug 3, 2018

clhunsen Aug 1, 2018

bockthom Aug 1, 2018

clhunsen Aug 1, 2018

clhunsen Aug 1, 2018

clhunsen Aug 1, 2018

clhunsen Aug 1, 2018

bockthom left a comment

bockthom Aug 1, 2018

clhunsen commented Aug 20, 2018

Improve add.vertex.attribute.first.activity #135

Improve add.vertex.attribute.first.activity #135

Conversation

klaraschlueter commented Jul 31, 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

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

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

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clhunsen commented Aug 20, 2018