search

uptake / pkgnet

R package for analyzing other R packages via graph representations of their dependencies
https://uptake.github.io/pkgnet/
Other
152 stars 37 forks source link

CICD Updates for Igraph changes and Ubuntu Tidy patch #306

Closed bburns632 closed 5 months ago

bburns632 commented 5 months ago

This PR gets CICD tests back to working status after some updates in rigraph and Ubuntu images caused job failures:

codecov-commenter commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 92.60%. Comparing base (9354e79) to head (25f6573).

:exclamation: Current head 25f6573 differs from pull request most recent head 399a4b3. Consider uploading reports for the commit 399a4b3 to get more accurate results

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #306 +/- ## ======================================= Coverage 92.60% 92.60% ======================================= Files 12 12 Lines 946 946 ======================================= Hits 876 876 Misses 70 70 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

bburns632 commented 5 months ago

That's a fair callout @jameslamb. I think we need to fix either roxygen or pkgdown to earlier versions (preferable to overhauling all our docs). Have any suggestions on versions to tie down in DESCRIPTION file?

jameslamb commented 5 months ago

I think it'd be better to use the latest version... it's hard to keep an == pin or ceiling working on CRAN indefinitely.

I could do it in a follow-up PR some time in the next few days if you'd like!

bburns632 commented 5 months ago

I got some time. I'll take a look. Trying out this hack first: https://github.com/r-lib/roxygen2/issues/457#issuecomment-198454631

bburns632 commented 5 months ago

OK. @jameslamb How about this? We just add Roxygen: list(r6 = FALSE) to the DESCRIPTION file to suppress the new formatting. I can't test on CRAN without resubmitting a new build (and I don't think this issue necessitates that), but this should resolve the issue. Roxygen is formatting our docs into a technically correct Rd that CRAN doesn't like. This is likely because we are not following the new format for R6 object docs (as we wrote them way before that). With this bit of code, it should format as it did before, with which CRAN had no issues.

I checked out the pkgdown build locally (mac) and it looked to be formatted fine with this parameter.

Let's let it fly.

jameslamb commented 5 months ago

Oh yeah I'm good with that! Ship it.

bburns632 commented 5 months ago

@jameslamb just need you to hit the approve button, and it'll be shipped. Blocked until then as I tagged you as a reviewer.

szhorvat commented 5 months ago
  • igraph updated their calculation of authorityScore in igraph release 0.10.5. Consequently, some expected values in tests needed to change.

Could you please clarify if you are seeing any change in results when computing the authority score of a directed graph? The fix in igraph 0.10.5 affected only undirected graphs (where hub and authority scores are identical to each other, as well as to the eigenvector centrality).

If you see a change on directed graphs, something may be wrong.

bburns632 commented 5 months ago

@szhorvat , I would appreciate your help investigating this potential issue. It is an directed graph.

Here is where we calculated the authority_score for a Directed Graph via igraph: https://github.com/uptake/pkgnet/blob/a904eca16cc8ec7dc5d9429eb1e154fa97934021/R/GraphClasses.R#L462-L468

Here is where we build the iGraph object: https://github.com/uptake/pkgnet/blob/a904eca16cc8ec7dc5d9429eb1e154fa97934021/R/GraphClasses.R#L251-L281

Here is how to reproduce the exact iGraph object for which we changed expected values in the recent PR (#306 tests/testthat/testdata/node_measures_baseballstats_FunctionReporter.csv). It is an R package build during testing: 

git clone https://github.com/uptake/pkgnet.git
cd pkgnet
git reset --hard a904eca16cc8ec7dc5d9429eb1e154fa97934021 # main branch after PR merge

R
install.packages(c("devtools","igraph"))
library(devtools)
library(igraph)

install_local(".") # pkgnet 
library(pkgnet)

install_local("inst/baseballstats")
library(baseballstats)

t <- CreatePackageReport("baseballstats")
t$FunctionReporter$graph_viz # to see the vizual
g <- t$FunctionReporter$pkg_graph # to get the igraph object

ig <- g$igraph
igraph::is_igraph(ig)
# True 

igraph::write_graph(ig, 'baseballstats_igraph.xml', "graphml")

EDIT: Revised above to rely on devtools::install_local()

szhorvat commented 5 months ago

Thank you for the detailed instructions. I am not really an R user, so this makes it much easier for me. However, when following the procedure of the last code block, I'm stuck at 

> t <- CreatePackageReport("baseballstats")
INFO [2024-04-09 17:42:53] Creating package report for package baseballstats with reporters: SummaryReporter, DependencyReporter, FunctionReporter
FATAL [2024-04-09 17:42:53] pkgnet could not find an installed package named 'baseballstats'. Please install the package first.
Error in log_fatal(msg) : 
  pkgnet could not find an installed package named 'baseballstats'. Please install the package first.

I tried devtools::install_github('https://github.com/troymoench/baseballstats.git'), but installation still fails. Could you give advice on how I can proceed?

Alternatively, perhaps you could export and upload this specific igraph object, and I can just work with this?

Hopefully there's nothing wrong, but I was surprised to see this PR and I want to make sure that there's no issue in igraph (no incorrect result).

jayqi commented 5 months ago

@szhorvat baseballstats is a dummy package we use for testing that is included in the source of this repo. You can find it here: https://github.com/uptake/pkgnet/tree/main/inst/baseballstats (the relative path should be consistent no matter what commit you're on)

My R is rusty and I'm not sure if this is the best way, but I think devtools::install or devtools::install_local to the local source should work.

bburns632 commented 5 months ago

@szhorvat My mistake. I thought referencing an internal script to set up the packages worked, but in reality, I still have some installed in my dev environment from this PR.

I edited the above to leverage devtools as @jayqi mentioned. Should work.

Also, here is the "graphml" file created from that script: baseballstats_igraph.txt

szhorvat commented 5 months ago

Thanks for the help! I now see what's going on. The change actually happened in 0.10.0, not in 0.10.5 (the latter affecting only undirected graphs, where hub and authority scores are non-interesting anyway).

The bad news:

The network you have is rather unfortunate, as the hub and authority scores are not uniquely defined with it. These scores are the leading eigenvector of $AA^T$ and $A^TA$, respectively. In this case the leading eigenvalue is degenerate, having a two-dimensional eigenspace. $(a, b, b, 0, 0)$ and $(0, a, a, b, 0)$ are a valid solution as hub and authority scores for any $a$ and $b$. For reference,

$$AA^T = \left( \begin{array}{ccccc} 2 & 0 & 0 & 0 & 0 \ 0 & 1 & 1 & 0 & 0 \ 0 & 1 & 1 & 0 & 0 \ 0 & 0 & 0 & 0 & 0 \ 0 & 0 & 0 & 0 & 0 \ \end{array} \right)$$

In these cases igraph cannot guarantee a specific result, so future CI failures are possible. The result may in principle depend on things like the BLAS/LAPACK/ARPACK versions used. It's basically out of our control.

The fix that was made in 0.10 was to ensure that the hub and authority scores are returned as a matching pair, as described here:

https://github.com/igraph/igraph/blob/4dcd230d2c0f4ab85f2784d69baa9ccce4e093a5/src/centrality/hub_authority.c#L149-L161

The good news:

szhorvat commented 5 months ago

The tl;dr is that we're (probably) good, no need to do anything for either pkgnet or igraph. A slight change of the network, if possible, would eliminate the degeneracy, but I wouldn't worry about it until you actually start to see inconsistent results.

Thanks for helping me examine this!

pkgnet is a cool package, it was interesting to apply it to igraph itself.

bburns632 commented 5 months ago

@szhorvat, thank you so much! If we notice any inconsistent errors, we'll adjust our test packages to avoid this issue altogether.

Thanks for the compliment. We obviously love igraph, and I had to try out a report of igraph as well. Very meta.