Fix igraph deprecation warnings as well as a bug in issue network construction #264

maxloeffler · 2024-05-28T10:51:17Z

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

Fix igraph deprecation warnings
Fix a bug in issue network construction that is detailed in Update deprecated function calls from other packages #260.

Changelog

Signed-off-by: Maximilian Löffler <[email protected]>

The bug, described in Issue#260, practially limits the amount of edges that can be present in an issue network to 17 and additionally leads to warnings in every case where there are not exactly 17 edges present. It is caused by an incorrect parameter to the 'base::split' function. We intend to pass a vector containing the numbers 1 to nrow(add.links), but instead pass 1 to 17. Signed-off-by: Maximilian Löffler <[email protected]>

codecov · 2024-05-28T11:14:33Z

Codecov Report

Attention: Patch coverage is 78.26087% with 15 lines in your changes missing coverage. Please review.

Project coverage is 80.24%. Comparing base (a87ff24) to head (3c2f910).

Files	Patch %	Lines
util-plot.R	0.00%	11 Missing ⚠️
util-bulk.R	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #264      +/-   ##
==========================================
+ Coverage   79.89%   80.24%   +0.34%     
==========================================
  Files          16       16              
  Lines        4905     4905              
==========================================
+ Hits         3919     3936      +17     
+ Misses        986      969      -17

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

In practice, GitHub is by far the most used data source for issues. By adjusting the default to 'github' only, we can greatly reduce the amount of raised warnings in production. Signed-off-by: Maximilian Löffler <[email protected]>

Signed-off-by: Maximilian Löffler <[email protected]>

bockthom · 2024-06-19T21:19:49Z

I've identified the reason for why the pipeline fails for R 3.6 now: Package testthat depends on package evaluate. A new version of package evaluate has been released a few days ago, and the most recent version of package evaluate depends on R 4.0. Thus, we would have to hard-code the previous version of package evaluate to be able to install testthat for R 3.6. As we try to avoid hard-coding package versions in coronet, this is the moment where should think about ceasing the support for R 3.6.

Independent of that, before we fix that issue, can we somehow enable the generation of the coverage report in our CI pipeline? Maybe it would be a good idea to generate the coverage report if the tests for the version, for which we generate the report, have finished successfully, even if the test pipeline fails for other versions? Just to avoid that the test coverage is not generated just because of specific issues in a single R version. What do you think @MaLoefUDS?

maxloeffler · 2024-06-19T21:24:05Z

That should be straightforward. I will look into it tomorrow 👍

maxloeffler · 2024-06-22T13:57:23Z

To make the upload-coverage-report job run, we need the build to finish successfully. The build job only counts as being passed if all its steps passed (i.e., for all R versions). I found multiple fixes, that implement your requested behavior. For example we could just add continue-on-error onto the run tests and run showcase steps. Then in any case when an older R version fails, we simply continue and for the latest R version, we still check in the generate coverage report step that the run tests step succeeded and nothing about the behavior would change. However, for this fix (and also for all other fixes I discovered) all build jobs will always display the pass symbol and never the red cross. There are indications about the internal failure but they are not as immanent.

So what is your preference? Currently, the only options I see is that we do not upload coverage if at least 1 R version did not run successfully (which may even make sense?) or that we still upload but then we only have the green check on the build status indicators.

bockthom · 2024-06-24T09:21:40Z

I don't like to have green check marks for failing steps. That'd be weird.
Then we should stay with the current version of the pipeline, I guess.
That is, we really need to fix every failing steps for old R versions immediately before merging PRs – otherwise the code coverage reports would be missing until the failing steps are fixed.
In our case, this means that we need to exclude 3.6 from the build pipeline, remove the corner-case handling for 3.6 in install.R, and update the README.md regarding supported R versions...

maxloeffler · 2024-06-24T13:24:57Z

Is that something you want me to do inside of this PR? 🤓

bockthom · 2024-06-24T13:33:57Z

We have two options: Either we open a new PR that fixes the CI pipeline, and rebase this PR after the other one is merged.
Or we directly address the CI issue here in this PR, which is completely legitimate since this PR deals with fixing deprecation issues 😉 What ever you'd prefer; I would be interested in your opinion on this.

Regarding how and when to update the README.md we should discuss potential solutions in our next meeting, this could be tricky as we differentiate between "supported" and "recommended" R versions. But fixing the CI pipeline and the deprecation workarounds in the package installation script is something that should be straightforward.

maxloeffler · 2024-06-24T15:09:58Z

I agree with you: since we are addressing deprecated features in here, I think it is legitimate to fix the CI here as well 👍

maxloeffler · 2024-06-26T10:00:59Z

Regarding the workarounds in the install script: What of the two workarounds are only present for R 3.6? As I see it, both of the workarounds may come to play also for newer but strangely configured or incorrectly installed R versions.

Context:

coronet/install.R

Lines 67 to 80 in 24005e4

    
           igraph.version = installed.packages()[rownames(installed.packages()) == "igraph", "Version"] 
        
           if (compareVersion(igraph.version, "1.3.0") == -1) { 
        
               print("WARNING: igraph version 1.3.0 or higher is recommended for using coronet.") 
        
           } 
        
           Matrix.version = installed.packages()[rownames(installed.packages()) == "Matrix", "Version"] 
        
           if (compareVersion(Matrix.version, "1.3.0") == -1) { 
        
               print("WARNING: Matrix version 1.3.0 or higher is necessary for using coronet. Re-install package Matrix...") 
        
               install.packages("Matrix", dependencies = NA, verbose = TRUE, quiet = TRUE) 
        
               Matrix.version = installed.packages()[rownames(installed.packages()) == "Matrix", "Version"] 
        
               if (compareVersion(Matrix.version, "1.3.0") == -1) { 
        
                   print("WARNING: Re-installation of package Matrix did not end up in the necessary packge version.") 
        
               } 
        
           }

bockthom · 2024-06-26T10:43:36Z

Regarding the workarounds in the install script: What of the two workarounds are only present for R 3.6?

Actually non of the ones in your quoted excerpt. The workaround is only present on the dev branch right now:

coronet/install.R

Lines 81 to 83 in a87ff24

    
           #install.packages("Matrix", dependencies = NA, verbose = TRUE, quiet = TRUE) 
        
           matrix.1.3.4.url = "https://cran.r-project.org/src/contrib/Archive/Matrix/Matrix_1.3-4.tar.gz" 
        
           install.packages(matrix.1.3.4.url, repos = NULL, dependencies = NA, verbose = TRUE, quiet = TRUE)

The other two version checks in your quoted excerpt are independent of R 3.6. The first one is because we want to support/recommend a minimum version number of igraph, and the other one regarding Matrix is because Matrix is part of the automatically installed packages of r-recommended (or even r-base) and needs to be re-installed to get a more recent package version than what comes with the R installation. This should be kept.
The only R3.6-specific change is the hard-coded Matrix version.

maxloeffler · 2024-06-26T10:45:23Z

Oh okay, I see 🤓

Deprecating 3.6 was decided in PR se-sic#264 because a subpackage of testthat now depends on R versions above 4.0. Signed-off-by: Maximilian Löffler <[email protected]>

.github/workflows/pull_request.yml

Deprecating 3.6 was decided in PR se-sic#264 because a subpackage of testthat now depends on R versions above 4.0. Signed-off-by: Maximilian Löffler <[email protected]>

Signed-off-by: Maximilian Löffler <[email protected]>

bockthom · 2024-06-27T07:32:22Z

Interesting: The upload of the coverage report worked again without changing anything, right?

bockthom

Thanks for this PR @MaLoefUDS. It looks really great, thanks for your efforts on changing all the reading and splitting tests due to the new default value of the issue source.

I spotted two minor issues:

the new default value is not updated in the README.md yet (see details below)
Codecov points us to some uncovered functionality. We should take advantage of this and add a few tests for some of the uncovered functions revealed by Codecov. But definitively not for all of them. I don't care about everything in util-bulk.R, and we don't have a proper testing setup for util-plot.R yet. So we can ignore Codecov's complaints regarding these files. But Codecov also revealed a small number of functions in well-tested files such as util-networks.R that seem to be untested still, and I would like to add some small tests for them. Please see my comments below for the particular functions I am referring to.

util-conf.R

bockthom · 2024-06-27T07:46:53Z

tests/test-networks.R

-    expect_true(igraph::are.connected(simplify.network(g), "A", "Base_Feature")) # specific edge
+    expect_true(igraph::are_adjacent(simplify.network(g), "A", "Base_Feature")) # specific edge


That's an interesting change, ending up in a completely different function name that has nothing in common with the previous name...
(Just a comment, nothing to worry about.)

util-misc.R

util-networks.R

Signed-off-by: Maximilian Löffler <[email protected]>

tests/test-misc.R

bockthom · 2024-07-02T11:05:38Z

Codecov Report

Attention: Patch coverage is 72.46377% with 19 lines in your changes missing coverage. Please review.

Project coverage is 79.89%. Comparing base (a87ff24) to head (fb3f547).

❗ Current head fb3f547 differs from pull request most recent head 0146a3b
Please upload reports for the commit 0146a3b to get more accurate results.

Files Patch % Lines
util-plot.R 0.00% 11 Missing ⚠️
util-bulk.R 0.00% 4 Missing ⚠️
util-networks.R 93.02% 3 Missing ⚠️
util-misc.R 0.00% 1 Missing ⚠️
Additional details and impacted files

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

@MaLoefUDS Do you have any idea why the Coverage report in this issue is not updated?
On the Codecov website, everything looks fine and everything is up to date. But here in the PR, the corresponding comment has been updated right after your commits today in the morning, but it still shows outdated information. Do you have any idea why this is the case?

maxloeffler · 2024-07-02T15:00:59Z

I do not know why the comment does not update. The docs state that the default behavior is to update the comment and otherwise send a new message. We did not update the default and there is no new comment, so we can assume that the codecov update happened and failed for some reason. We can force codecov to send new comments on every change to the PR, maybe that would help, but im unsure. Apart from that I have no idea 🤷‍♂️

bockthom · 2024-07-02T15:38:47Z

Right now the comment from Codecov is up to date again and refers to the most recent commit (see quote below). So I guess something got stuck there (for whatever reason). So let's assume it works well.

Codecov Report

Attention: Patch coverage is 76.81159% with 16 lines in your changes missing coverage. Please review.

Project coverage is 80.22%. Comparing base (a87ff24) to head (07a06cd).

Files Patch % Lines
util-plot.R 0.00% 11 Missing ⚠️
util-bulk.R 0.00% 4 Missing ⚠️
util-networks.R 97.67% 1 Missing ⚠️
Additional details and impacted files

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

bockthom · 2024-07-02T15:46:02Z

We can force codecov to send new comments on every change to the PR

I don't think that @hechtlC would agree with that 😉 We already receive lots of e-mails, we don't need to receive multiple e-mails for one push event... If codecov is able to properly update the first comment (as it seems to be, at the moment), updating the comment is preferred over sending new comments, I guess.

tests/test-misc.R

Signed-off-by: Maximilian Löffler <[email protected]>

bockthom

Everything looks good to me now. So, the only thing that is missing is to add the changes to the NEWS.md.

hechtlC

Looks good to me! Thanks @MaLoefUDS .

NEWS.md

Signed-off-by: Maximilian Löffler <[email protected]>

Deprecating 3.6 was decided in PR se-sic#264 because a subpackage of testthat now depends on R versions above 4.0. Signed-off-by: Maximilian Löffler <[email protected]>

maxloeffler added 2 commits May 28, 2024 12:45

Replace recently deprecated igraph methods to mitigate warnings

0df9d5b

Signed-off-by: Maximilian Löffler <[email protected]>

maxloeffler added 2 commits June 19, 2024 15:56

Make 'github' the default data source for issues

5ff83c3

In practice, GitHub is by far the most used data source for issues. By adjusting the default to 'github' only, we can greatly reduce the amount of raised warnings in production. Signed-off-by: Maximilian Löffler <[email protected]>

Adjust tests to comply with the new default for issue sources

8c8080c

Signed-off-by: Maximilian Löffler <[email protected]>

bockthom reviewed Jun 26, 2024

View reviewed changes

.github/workflows/pull_request.yml Outdated Show resolved Hide resolved

maxloeffler force-pushed the dev branch from 5f15ad8 to 3ba9ac4 Compare June 26, 2024 10:56

Remove support for R version 3.6 from CI and install script

c8e6f45

Deprecating 3.6 was decided in PR se-sic#264 because a subpackage of testthat now depends on R versions above 4.0. Signed-off-by: Maximilian Löffler <[email protected]>

maxloeffler force-pushed the dev branch from 3ba9ac4 to c8e6f45 Compare June 26, 2024 11:23

Adjust minimal and recommended R version to exclude 3.6

fb3f547

Signed-off-by: Maximilian Löffler <[email protected]>

bockthom requested changes Jun 27, 2024

View reviewed changes

Improve consistency of 'issues.from.source' parameter

8bcbc81

Signed-off-by: Maximilian Löffler <[email protected]>

bockthom reviewed Jul 2, 2024

View reviewed changes

tests/test-misc.R Outdated Show resolved Hide resolved

maxloeffler force-pushed the dev branch from 0146a3b to 07a06cd Compare July 2, 2024 14:51

bockthom reviewed Jul 3, 2024

View reviewed changes

tests/test-misc.R Outdated Show resolved Hide resolved

maxloeffler added 2 commits July 3, 2024 19:45

Add tests for uncovered functionality in 'util-misc' and 'util-networks'

ff30f32

Signed-off-by: Maximilian Löffler <[email protected]>

Test simplification of networks with graph attributes

af80551

Signed-off-by: Maximilian Löffler <[email protected]>

maxloeffler force-pushed the dev branch from 6d039d6 to af80551 Compare July 3, 2024 17:45

bockthom reviewed Jul 4, 2024

View reviewed changes

hechtlC previously approved these changes Jul 5, 2024

View reviewed changes

maxloeffler dismissed hechtlC’s stale review via e8ff999 July 6, 2024 06:52

bockthom reviewed Jul 6, 2024

View reviewed changes

NEWS.md Outdated Show resolved Hide resolved

maxloeffler force-pushed the dev branch from e8ff999 to c4079d6 Compare July 6, 2024 07:59

bockthom reviewed Jul 6, 2024

View reviewed changes

NEWS.md Outdated Show resolved Hide resolved

Update 'NEWS.md'

3c2f910

Signed-off-by: Maximilian Löffler <[email protected]>

maxloeffler force-pushed the dev branch from c4079d6 to 3c2f910 Compare July 6, 2024 08:14

bockthom approved these changes Jul 6, 2024

View reviewed changes

bockthom merged commit 74ebe0b into se-sic:dev Jul 8, 2024
8 of 9 checks passed

bockthom mentioned this pull request Feb 2, 2025

Version 5.0 #280

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix igraph deprecation warnings as well as a bug in issue network construction #264

Fix igraph deprecation warnings as well as a bug in issue network construction #264

maxloeffler commented May 28, 2024 •

edited by bockthom

Loading

codecov bot commented May 28, 2024 •

edited

Loading

bockthom commented Jun 19, 2024

maxloeffler commented Jun 19, 2024

maxloeffler commented Jun 22, 2024

bockthom commented Jun 24, 2024

maxloeffler commented Jun 24, 2024

bockthom commented Jun 24, 2024

maxloeffler commented Jun 24, 2024

maxloeffler commented Jun 26, 2024

bockthom commented Jun 26, 2024 •

edited

Loading

maxloeffler commented Jun 26, 2024

bockthom commented Jun 27, 2024

bockthom left a comment

bockthom Jun 27, 2024

bockthom commented Jul 2, 2024

Codecov Report

maxloeffler commented Jul 2, 2024

bockthom commented Jul 2, 2024

Codecov Report

bockthom commented Jul 2, 2024

bockthom left a comment

hechtlC left a comment

		expect_true(igraph::are.connected(simplify.network(g), "A", "Base_Feature")) # specific edge
		expect_true(igraph::are_adjacent(simplify.network(g), "A", "Base_Feature")) # specific edge

Fix igraph deprecation warnings as well as a bug in issue network construction #264

Fix igraph deprecation warnings as well as a bug in issue network construction #264

Conversation

maxloeffler commented May 28, 2024 • edited by bockthom Loading

Prerequisites

Description

Changelog

codecov bot commented May 28, 2024 • edited Loading

Codecov Report

bockthom commented Jun 19, 2024

maxloeffler commented Jun 19, 2024

maxloeffler commented Jun 22, 2024

bockthom commented Jun 24, 2024

maxloeffler commented Jun 24, 2024

bockthom commented Jun 24, 2024

maxloeffler commented Jun 24, 2024

maxloeffler commented Jun 26, 2024

bockthom commented Jun 26, 2024 • edited Loading

maxloeffler commented Jun 26, 2024

bockthom commented Jun 27, 2024

bockthom left a comment

Choose a reason for hiding this comment

bockthom Jun 27, 2024

Choose a reason for hiding this comment

bockthom commented Jul 2, 2024

Codecov Report

maxloeffler commented Jul 2, 2024

bockthom commented Jul 2, 2024

Codecov Report

bockthom commented Jul 2, 2024

bockthom left a comment

Choose a reason for hiding this comment

hechtlC left a comment

Choose a reason for hiding this comment

maxloeffler commented May 28, 2024 •

edited by bockthom

Loading

codecov bot commented May 28, 2024 •

edited

Loading

bockthom commented Jun 26, 2024 •

edited

Loading