Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 10 commits into from
Jul 8, 2024

Conversation

maxloeffler
Copy link
Contributor

@maxloeffler maxloeffler commented May 28, 2024

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

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]>
Copy link

codecov bot commented May 28, 2024

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]>
@bockthom
Copy link
Collaborator

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
Copy link
Contributor Author

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

@maxloeffler
Copy link
Contributor Author

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.

Bildschirmfoto 2024-06-22 um 15 54 36

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
Copy link
Collaborator

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
Copy link
Contributor Author

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

@bockthom
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Collaborator

bockthom commented Jun 26, 2024

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
Copy link
Contributor Author

Oh okay, I see 🤓

maxloeffler added a commit to maxloeffler/coronet that referenced this pull request Jun 26, 2024
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 a commit to maxloeffler/coronet that referenced this pull request Jun 26, 2024
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]>
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]>
@bockthom
Copy link
Collaborator

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

Copy link
Collaborator

@bockthom bockthom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

@bockthom
Copy link
Collaborator

bockthom commented Jul 2, 2024

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
Copy link
Contributor Author

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
Copy link
Collaborator

bockthom commented Jul 2, 2024

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
Copy link
Collaborator

bockthom commented Jul 2, 2024

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.

Copy link
Collaborator

@bockthom bockthom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

hechtlC
hechtlC previously approved these changes Jul 5, 2024
Copy link
Contributor

@hechtlC hechtlC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thanks @MaLoefUDS .

Signed-off-by: Maximilian Löffler <[email protected]>
@bockthom bockthom merged commit 74ebe0b into se-sic:dev Jul 8, 2024
8 of 9 checks passed
Leo-Send pushed a commit to Leo-Send/coronet that referenced this pull request Nov 13, 2024
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]>
@bockthom bockthom mentioned this pull request Feb 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants