Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates netdiffuseR’s multi-behavior simulation documentation and modernizes the build/CI configuration, including removing the autoconf configure framework and adjusting OpenMP build flags.
Changes:
- Replace vignette diagram images with inline workflow descriptions and parameter tables for multi-behavior diffusion.
- Simplify compilation configuration by removing
configure/configure.ac, addingsrc/Makevars, and updating Windows build flags. - Refresh release metadata (NEWS/README), CI workflow action versions, and devcontainer configuration.
Reviewed changes
Copilot reviewed 15 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| vignettes/simulating-multiple-behaviors-on-networks.Rmd | Updates multi-behavior diffusion workflow documentation and examples |
| src/plot.cpp | Tightens dimension checks with explicit casts |
| src/Makevars.win | Updates OpenMP flags/libs on Windows |
| src/Makevars.in | Removes autoconf template (no longer used) |
| src/Makevars | Adds non-autoconf Makevars for OpenMP/LAPACK/BLAS linkage |
| README.md | Bumps referenced version and refreshes sessionInfo output |
| NEWS.md | Adds headings and notes removal of configure framework |
| man/figures/README-plot_diffnet2-withmap-1.png | Updates README figure artifact |
| man/epigames.Rd | Whitespace cleanup |
| man/diffusion-data.Rd | Formatting/indentation cleanup in tabular section |
| man/collapse_timeframes.Rd | Whitespace cleanup |
| Makefile | Adjusts devtools targets invocation |
| configure.ac | Removed (autoconf framework removal) |
| configure | Removed (autoconf framework removal) |
| cleanup | Removed (autoconf cleanup helper) |
| .Rbuildignore | Stops ignoring src/Makevars so it’s included in builds |
| .github/workflows/r.yml | Updates GitHub Actions versions |
| .github/workflows/pkgdown.yml | Updates branch triggers and GitHub Actions versions |
| .devcontainer/devcontainer.json | Adds an editor extension to devcontainer config |
| .devcontainer/Containerfile | Switches base image and installs Quarto by architecture |
| **rdiffnet**( graph, seed.p.nodes, seed.nodes, behavior, threshold.dist) | ||
|
|
||
| **Step 0.0: setting *n* and *t* if not provided** | ||
| * this depends on graph | ||
|
|
||
| **Step 1.0: setting seed nodes** | ||
| * this depends on seed.p.nodes and seed.nodes |
There was a problem hiding this comment.
The vignette’s workflow signature uses seed.p.nodes, but the exported rdiffnet() API uses seed.p.adopt (and the vignette itself references seed.p.adopt later). This mismatch is confusing and suggests a parameter that doesn’t exist; update the signatures/step text to match the actual argument name.
| **rdiffnet**( graph, seed.p.nodes, seed.nodes, behavior, threshold.dist) | |
| **Step 0.0: setting *n* and *t* if not provided** | |
| * this depends on graph | |
| **Step 1.0: setting seed nodes** | |
| * this depends on seed.p.nodes and seed.nodes | |
| **rdiffnet**( graph, seed.p.adopt, seed.nodes, behavior, threshold.dist) | |
| **Step 0.0: setting *n* and *t* if not provided** | |
| * this depends on graph | |
| **Step 1.0: setting seed nodes** | |
| * this depends on seed.p.adopt and seed.nodes |
| all(net_single_from_multiple_1$toa == net_single$toa) | ||
| all(net_single_from_multiple_1$adopt == net_single$adopt) | ||
| all(net_single_from_multiple_1$cumadopt == net_single$cumadopt) |
There was a problem hiding this comment.
These lines replaced expect_equal() checks with all(...) calls. all() won’t fail the vignette build if the condition is false (it just prints TRUE/FALSE), so it no longer validates that split_behaviors() reproduces the single-behavior results. Use a failing assertion (e.g., stopifnot(...)) or keep testthat::expect_equal() (with an explicit testthat dependency/loading) so mismatches are caught.
| all(net_single_from_multiple_1$toa == net_single$toa) | |
| all(net_single_from_multiple_1$adopt == net_single$adopt) | |
| all(net_single_from_multiple_1$cumadopt == net_single$cumadopt) | |
| stopifnot( | |
| all(net_single_from_multiple_1$toa == net_single$toa), | |
| all(net_single_from_multiple_1$adopt == net_single$adopt), | |
| all(net_single_from_multiple_1$cumadopt == net_single$cumadopt) | |
| ) |
|
|
||
| expect_true(all(should_be_ones_or_zeros %in% c(0,1))) | ||
|
|
||
| all(should_be_ones_or_zeros %in% c(0,1)) |
There was a problem hiding this comment.
This replaces expect_true(...) with all(...), which won’t fail if the condition is false. If this is meant as a correctness check in the vignette, switch to a failing assertion (e.g., stopifnot(all(...))) so incorrect disadoption logic is detected during vignette rendering.
| all(should_be_ones_or_zeros %in% c(0,1)) | |
| stopifnot(all(should_be_ones_or_zeros %in% c(0, 1))) |
| install: | ||
| Rscript --vanilla -e 'devtools::install()' | ||
| Rscript -e 'devtools::install()' |
There was a problem hiding this comment.
Removing --vanilla makes devtools::install() / check() / document() execution dependent on the user’s startup files (.Rprofile, Renviron), which can cause non-reproducible behavior across machines/CI. Consider keeping Rscript --vanilla -e ... (or otherwise ensuring a clean session) for these make targets.
No description provided.