Base active ranges computation on multiple data sources and adapt first activity #160
Conversation
There was a problem hiding this comment.
Thanks for your pull request, @klaraschlueter.
It looks really good, even if I did not yet comprehend all the fancy functions in detail.
However, I already identified some minor issues and questions. Please have a look at them.
There was a problem hiding this comment.
Thank you very much for your work, @klaraschlueter. I see that this is quite some complex stuff you are dealing with. 😲 I was not aware of this in that detail...
I have annotated some minor issues. Additionally, we need to talk about the function list.by.inner.level in more detail and probably in a face-to-face meeting. Unfortunately, I do not get all the details yet.
klaraschlueter
force-pushed
the
klara-updates
branch
from
82784da to
b57d408
Compare
The function add.vertex.attribute.active.ranges is adapted. It is possible now to call it with a vector of multiple activity types. The lists containing active ranges per type and author as before are combined to a list containing an element for every given activity type and added as vertex attribute. Signed-off-by: Klara Schlueter <[email protected]>
Signed-off-by: Klara Schlueter <[email protected]>
Move functionality of compute.attr from add.vertex.attribute.active.ranges to the new funciton get.active.ranges.data (refactoring). Move the match.arg.or.default call for the activity.types from add.vertex.attribute.first.activity to get.first.activity.data for internal consistency and analogy between first activity and active ranges. Signed-off-by: Klara Schlueter <[email protected]>
Signed-off-by: Klara Schlueter <[email protected]>
The innermost list in vertex attributes computed by
add.vertex.attribute.active.ranges contains ranges as Strings. This range list
was named with these same range describing Strings, e.g. list("range1" =
"range1", "range2" = "range2"). This commit removes the names.
Signed-off-by: Klara Schlueter <[email protected]>
Add function get.expected.active.ranges to return expected active.ranges data and call in test. Signed-off-by: Klara Schlueter <[email protected]>
The function add.vertex.attribute.first.activity already provided a parameter default.value. This parameter is now interpreted as default per person and data source and the function get.first.activity.data is adapted to use it properly. Signed-off-by: Klara Schlueter <[email protected]>
If first activity computation is done per data source (instead of over all data sources), construct the default value for the whole vertex attribute as a list of the same structure as given attributes, containing as elements the default value per author and data source. Signed-off-by: Klara Schlueter <[email protected]>
If active ranges computations is done per data source (instead of over all data sources), construct the default value for the whole vertex attribute as a list of the same structure as given attributes, containing as elements the default value per author and data source. Signed-off-by: Klara Schlueter <[email protected]>
The functions add.vertex.attribute.first.activity and add.vertex.attribute.active.ranges both accept a parameter default.value. This given value is interpreted as default value per type and author (not as default attribute for vertices for which no data is available). It is distinguished from the constructed vertex default and added to the vertex attributes in the corresponding helper functions. Signed-off-by: Klara Schlueter <[email protected]>
The test verifies that the function add.active.ranges inserts the expected default values both for vertices (author unknown in active ranges data) and for active ranges (missing information per known author and activity type). Signed-off-by: Klara Schlueter <[email protected]>
The flag combine.all.activities of the function add.vertex.attribute.active.ranges indicates, if the vertex attribute should contain a list of ranges per data source or one list for all data sources. The functionality for the second possibility is implemented in this commit. Signed-off-by: Klara Schlueter <[email protected]>
Test the functionality introduced in 674372b1ee4d2733e6d9c471efdecf42b470d2e3. Signed-off-by: Klara Schlueter <[email protected]>
Refactor and adapt active ranges and first activity. Signed-off-by: Klara Schlueter <[email protected]>
Signed-off-by: Klara Schlueter <[email protected]>
Signed-off-by: Klara Schlueter <[email protected]>
Add spaces between "if" and "(", add documentation for default values,
clarify example for finding minima in first activity computation.
Signed-off-by: Klara Schlueter <[email protected]>
Apply documentation conventions and give input-output example for list.by.inner.level. Signed-off-by: Klara Schlueter <[email protected]>
..and remove unused parameter from helper function. Signed-off-by: Klara Schlueter <[email protected]>
Signed-off-by: Klara Schlueter <[email protected]>
klaraschlueter
force-pushed
the
klara-updates
branch
from
0b64b3a to
eb4bd4c
Compare
There was a problem hiding this comment.
Hi @klaraschlueter,
thank you very much for your patience and endurance! The PR looks fine for me, I just have suggestions for very minor refactorings and renamings in the tests that you added.
This is the last iteration from my side. 😀👍
Signed-off-by: Klara Schlueter <[email protected]>
There was a problem hiding this comment.
Thanks @klaraschlueter. This PR really contains a nicely implemented and useful feature. Thank you also for your endurance regarding finding appropriate function names 😉
I have no further suggestions regarding the functionality and implementation details of this PR. Looks good now!
Nonetheless, there is one minor issue which needs to be fixed before merging:
During rebasing, commit hashes change. As you have rebased today, the commit hashes in the changelog file "NEWS.md" are not valid any more. Could you please update them (see comments below.)? Thanks a lot! As soon as those are updated, I will merge this PR.
Signed-off-by: Klara Schlueter <[email protected]>
Prerequisites
dev.Description
Adresses issue #92: The function add.vertex.attribute.active.ranges is complemented by the possibility to base the computation on multiple data sources. The flag "combine.all.activity.types" is introduced, indicating if the generated vertex attribute contains one list per data source or a single list for all data sources. The default value in both first activity and active ranges is used as default per author and type. The default appended as vertex attribute is constructed from the given default analogously to the vertex attributs actually containing data.
Changelog
Added
674372b)
Changed/Improved