Integrate subissues and agent handling

[Leo-Send]

Subissues are now edges in an issue network, agents can be filtered

Prerequisites

• I adhere to the coding conventions (described here) in my code.
• I have updated the copyright headers of the files I have modified.
• I have written appropriate commit messages, i.e., I have recorded the goal, the need, the needed changes, and the location of my code modifications for each commit. This includes also, e.g., referencing to relevant issues.
• I have put signed-off tags in all commits.
• I have updated the changelog file NEWS.md appropriately.
• I have checked whether I need to adjust the showcase file showcase.R with respect to my changes.
• The pull request is opened against the branch dev.

Description

Changelog

[Leo-Send]

For now, this allows us to use the subissue data for network construction and filter agents analogue to bots. I will check if there are other places where bots get special treatment, and see if we want to also include agents there.

[Copilot]

Pull request overview

This PR extends coronet’s issue/network and filtering capabilities by (1) treating sub-issues as edges in the issue artifact network and (2) adding first-class handling for “agents” alongside bots, including a new configuration flag to filter them from datasets.

Changes:

• Add parsing/handling intended to turn sub-issue relationships into issue-network edges.
• Introduce filter.agents configuration and propagate it through commits/issues/mails filtering.
• Extend bot/author metadata to include is.agent.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File	Description
util-read.R	Updates issue parsing and expands bot/author metadata with is.agent.
util-networks.R	Adds construction of issue-network edges for sub-issue events.
util-data.R	Wires new filter.agents flag through ProjectData filtering and adds filter.agents() implementation.
util-conf.R	Adds new filter.agents ProjectConf attribute.

Comments suppressed due to low confidence (1)

util-read.R:476

• The roxygen for read.bot.info() says it returns a boolean for whether an author is a bot, but the data now includes is.agent as well. Please update the function documentation to reflect the new column so users know agents are supported.

#' Read the bot classification from the 'bots.list' file.
#'
#' @param data.path the path to the commit-messages list
#'
#' @return a data frame with author.name, author.email, and a (potentially NA) boolean whether this is a bot,
#'         or \code{NULL} if the above file is not present.
read.bot.info = function(data.path) {

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov]

Codecov Report

❌ Patch coverage is 95.83333% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.76%. Comparing base (281c917) to head (653d02a).

Files with missing lines	Patch %	Lines
util-data.R	90.47%	2 Missing ⚠️
util-read.R	95.83%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #292      +/-   ##
==========================================
+ Coverage   82.59%   82.76%   +0.17%     
==========================================
  Files          16       16              
  Lines        5377     5443      +66     
==========================================
+ Hits         4441     4505      +64     
- Misses        936      938       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[bockthom]

@Leo-Send Could you please check the review commits from Copilot from March 18? Seems like some of them have not been addressed yet or discussed yet... [The one regarding components supposes a way too complex change - maybe we can just check whether this is an GitHub issue or not and only perform the conversion if its from GitHub]. Regarding the other ones, I am not sure, please check. Some of the other comments seem necessary to be addressed to me. In addition, I will ask Copilot for a new review now to assess the new changes - I did not spot any mistakes, but let's see what Copilot says about the tests.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.

[Leo-Send]

I do not understand why, but since my latest rebase the core/periphery classification on R version 4.5 seems to fail/behave differently. If I remember correctly, it passed before, and I did not change anything about the test since then. I will investigate locally, although changing the tests is not really an option since it would break all the other versions.

[bockthom]

I had a quick look at the logs. The failing test actually is a test we already had trouble with in the past as it depends on floating point precision when computing eigenvector centrality. We tried to implement the test in a way that it is independent of such precision variations - but here one peripheral dev seems to become a core dev. I don't know why. Do you have a running version of R 4.5 to check what eigenvector values we get there?

I have an installation of R 4.5.3 and I don't get this error when running the tests (but I get 6 different errors on empty networks, maybe a bug in the older igraph version I use - i will investigate this later whether the 6 other errors disappear when I switch to the most recent igraph version).

As for the core-periphery test fail: If we cannot reproduce it locally, could you please add some logs / debug prints that show us the respective values in the CI logs?

[Leo-Send]

While I managed to get everything to run on an upgraded R version on my machine, I cannot replicate the error. Therefore, I am adding a debug print statement.

[Leo-Send]

So now, the test fails for R 4.3. The issue lies in calculating the eigenvektor centrality for the most peripheral developers. The concrete values we describe in the test are not correct at all, but they lie in the variance (since we accept a variance of up to 0,0001 and the values are several orders of magnitude smaller than than with 10^-19 in the actual results and 10^-17 in the number mentioned in the test.

It is unclear to me how I could make this test less flaky without modifying the test or the data significantly. One option could be to limit the number of developers the classification considerd using the 'results.limit' function parameter. I will investigate this now.

Edit: I guess this just made it more flaky?

[bockthom]

Hm, I am not sure what to do here. So the problems are not differences in centrality values (we could be more lenient on them) - but the difficulty is that depending on these differences, developer Thomas sometimes is considered core and sometimes peripheral. This was a problem in the initial case, and after you changed the test, this is still a problem. But what changes after 4.3 is the centrality value beyond the top 3: From 3.610^-19 to 6.810^-19. As the centrality values of them almost double, Thomas is considered central enough be core although having the same centrality value in both cases. I don't know where these differences come from, seems eigenvector calculation has been changed in R 4.4.

The current result is worse than the previous one, as more versions are affected (previously it was exactly one), and the more recent ones are affected now, which is also bad.

I asked Gemini, and it pointed me to a change in R 4.4 where new LAPACK/BLAS versions have been introduced: https://stat.ethz.ch/pipermail/r-announce/2024/000701.html

Could you tell us a little bit more about how the network looks like? Maybe just plot this particular network and paste it here in the issue? Maybe disconnected subgraphs cause these problems. Otherwise, these 10^-19 values are numeric measurement noise and should all be zero. Couldn't we just treat all values smaller than 10^-10 as 0? Then the test should not be flaky any more. But this is something that we would need to adjust in the eigenvector classification where we compute eigenvector centralities. This should solve the problem entirely. But potentially we might need to fix a few more tests then.

[Leo-Send]

The network does indeed contain entirely isolated nodes. From my understanding, these should have a centrality value of 0, at least for the purposes of eigenvektor classification. I see two ways of adressing this issue:

1. We set an arbitrary boundary, for example 10^-10, and set all values below it to 0. This would solve the problem in this case, but could lead to incorrect classifications in very large networks were non-isolated nodes reach such low centrality values.
2. We go over all nodes in the network and set all centrality values of nodes without outgoing edges to 0. This should lead to more sound results, but could be computationally expensive.

[Leo-Send]

I quickly implemented both options and they seem to work. We also need to think about for which centrality metrics we want to do option 2, since I noticed that at least one metric (pagerank) has relatively high centrality values for isolated vertices, in this case with a value of around 0.04.

[hechtlC]

I quickly implemented both options and they seem to work. We also need to think about for which centrality metrics we want to do option 2, since I noticed that at least one metric (pagerank) has relatively high centrality values for isolated vertices, in this case with a value of around 0.04.

The issue for PageRank is actually intended behavior so this should not be a problem as the connected nodes should always have large enough centrality values. As for the rest: I see merits and problems with both methods. If we choose the small threshold, we need to have a separate case for eigenvector centrality which would hurt consistency but would make the other cases easier. If we were to detect the isolated nodes to manually set them to 0, we could potentially run into performance issues but we would treat every method the same.

I personally prefer the first method but am open to other suggestions. What do you think @bockthom ?

[bockthom]

Thank you @Leo-Send for implementing both options. Are we sure that all the "wrong" values that led to the test failure in the CI run are caused by isolates? If yes, than I would agree with the second solution. It should not be that computationally expensive - if you do a dataframe vector operation (e.g., centralities[[is.isolated(V(network)[[centralities]])]] = 0; pseud code, does not work) instead of a loop and use an igraph function (or function that we already have implemented) to find isolates instead of computing them ourselves - any kind of loop would be way too expensive.

Otherwise, I would go with the first method.

[Leo-Send]

Yes, the centrality values that changed between the versions all belong to isolated vertices. I did implement it using the degree function igraph has built in, but I will check if there is an even better igraph function for it.
What should I do with the pagerank classification? With this method, isolated vertices would also gain a centrality value of 0, even though pagerank assigns them a value I have observed to be between 0.05 and 0.001, which is much closer to 1 that the cases we have been looking at until now. Should I just exclude pagerank from this? Or do we just ignore it and set it to 0 anyway, since isolated vertices will still be classified as peripheral?

[hechtlC]

Yes, the centrality values that changed between the versions all belong to isolated vertices. I did implement it using the degree function igraph has built in, but I will check if there is an even better igraph function for it. What should I do with the pagerank classification? With this method, isolated vertices would also gain a centrality value of 0, even though pagerank assigns them a value I have observed to be between 0.05 and 0.001, which is much closer to 1 that the cases we have been looking at until now. Should I just exclude pagerank from this? Or do we just ignore it and set it to 0 anyway, since isolated vertices will still be classified as peripheral?

For PageRank it should not make a difference. In PageRank nodes without a connection always have the same small value and this value is always smaller than any node with connections. Therefore, in my opinion it does not matter whether this value is small or 0. What do you think @bockthom ?

[bockthom]

In page rank, isolated values should not have a value of zero - by definition.

I guess the problem is really eigenvector specific here and should only be fixed for eigenvector classification. Or do we have any other classification that leads to problems? We should handle this separately per classification method, as each classification methods comes with its own corner cases and specific implementations.

[hechtlC]

I am not entirely sure but I believe that hierarchy values can also get as small.

[bockthom]

Hierarchy values should not be a problem - as we just compute degree and clustering coefficient - none of them should lead to any numeric issues in case of isolates.

[Leo-Send]

Hierarchy values also use the degree as the numerator, so an isolated vertex will have a centrality of 0.

I just pushed the fix to test it on all versions. If everything passes, I will remove the filtering in the test runs, allowing the checks to run all tests again.

[hechtlC]

You are right @bockthom ! I forgot that we are talking about isolates. But if we were using the threshold cutoff, then it could be a problem.

[bockthom]

You are right @bockthom ! I forgot that we are talking about isolates. But if we were using the threshold cutoff, then it could be a problem.

Nope. Clustering coefficient is clearly defined - there should not be a numeric issue. The numeric issue only comes from the LAPACK/BLAS packages that are involved in eigenvector decompositions of matrices. Clustering coefficients are just fractions, and degrees are just integers. So, no problem to be expected for hierarchy.

[bockthom]

@Leo-Send Could you please rebase and remove the commits you added for debugging reasons?

Especially this one introduced wrong behavior:
Modify test for flakiness
Removing the changes from that commit should solve the problem.

[Leo-Send]

Removing the changes from that commit should solve the problem.

Oh, good catch! I will do that right now.

[bockthom]

Didn't it work before you last rebase? What did you change in your last rebase?

[Leo-Send]

I just removed the commit that limited the test run to only the core-peripheral tests, since everything was working. I do not understand why this fails again now...

[Leo-Send]

So now, R version 4.6 fails, the same that runs on my machine. Is it possible for you to rerun just the 'Build (latest)' part if the pipeline?

Actually, I will just reintroduce the filter and print statement. No need to rerun manually.

[hechtlC]

@Leo-Send we just discussed the issue again and tried to find where the test fails.
We came to the conclusion that it can only be a numeric flaky error in the centrality value calculation.

This is not something we can reasonably fix in the implementation. So our proposal is that you add one edge between a previously isolated and one of the two equal nodes (e.g., between Udo and Thomas) in this one test. So not in the test data but just in the network you have given in the test.

This should fix the flakiness without disrupting the other tests or the implementation.

[Leo-Send]

Alright. Although I am at this point quite certain where this flakiness occurs, I will just quickly validate my theory before I change the network.

ETA: As I thought, the issue is in the line core.classification = author.data[[calc.base.name]] > author.class.threshold or in the previous threshold calculation which somehow considers one of two identical variables to be greater than the other. I would assume this is a machine/processor dependent issue, since it inconsistently appears at different R versions and I fail to reproduce it locally.

Should I still keep the prior workaround to set all isolated centrality scores to 0, or should I remove that since we solve the issue another way?

[Leo-Send]

Adding the edge ("udo", "Thomas") makes it even more flaky. I now get different centrality scores on every test run locally. The same is true when I also add the inverse edge.

I tried a few different edges, each with the same result. as soon as I add an edge manually via igraph::add.edges, the results are all over the place and do not make any sense.

[bockthom]

Sorry for the late response @Leo-Send. I was at the beginning of a vacation when you had added your last two comments here.

For the first question: Yes, we want eigen-vector centrality to be 0 for isolates. Everything else would just be a measurement error. So, it should always be 0.

For adding the ("udo", "Thomas") edge: Could you please just push the change you made such that we can see the centrality values in your debug print statement in the CI? Or provide us with the exact centrality values of each developer from different test runs? Without knowing what's going on it is difficult to respond to that or debug it ourselves. If I'd have more concrete information what you did and how it changed the results, I could think about it or even debug it. Thank you!