-
Notifications
You must be signed in to change notification settings - Fork 22
Add degree and time of adoption diagnostic function #57
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
base: master
Are you sure you want to change the base?
Conversation
|
@gvegayon 👋 This repository doesn't have Copilot instructions. With Copilot instructions, I can understand the repository better, work faster and produce higher quality PRs. I can generate a .github/copilot-instructions.md file for you automatically. Click here to open a pre-filled issue and assign it to me. I'll write the instructions, and then tag you for review. |
Co-authored-by: gvegayon <[email protected]>
Co-authored-by: gvegayon <[email protected]>
|
@aoliveram, can you take a look at this (if you have the time and interest), and try to see if it works? Checkout this branch and give it a try. We may be able to add this to the next release of netdiffuseR. |
|
Awesome. @gvegayon yes, of course, I can look at the new function and tests. |
- Expanded degree_adoption_diagnostic to accept generic graphs + TOA - Improved print method with CI-aware interpretation - Updated tests for new input validation
|
Hey @gvegayon, I was checking all, and it looks pretty good! I added some improvements to accept graphs that are not diffnet objects (so they need TOA input). Also, some changes were made to the printer, so it is more specific now. The tests were updated to fit those modifications. Maybe it still could be enhaced to allow multi-diffusion objects. Tell me what do you think. |
Thanks! If you have the time and will, that'd be great. If not, that's OK. I would create an issue for the future. Also, R CMD check is not passing for Ubuntu-release. I'll try to re-run it and see what's going on. |
Sure @gvegayon, I can do something during the day ;) |
…t, safer bootstrap & printer; tests + docs added
|
Hey @gvegayon, I forgot to comment yesterday, but I added the
The thing is that the check for Ubuntu-release still fails. Do you know what could be happening? |
gvegayon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK @copilot, please address my comments. One thing you missed: update the NEWS file to include the new function.
attn @aoliveram
R/stats.R
Outdated
| #' @param graph A \code{\link{diffnet}} object or a graph data structure (classes include | ||
| #' \code{array} (\eqn{n\times n \times T}{n*n*T}), \code{dgCMatrix} (sparse), | ||
| #' \code{igraph}, etc.; see \link{netdiffuseR-graphs}). | ||
| #' @param toa Integer vector of length \eqn{n} (single behavior) or an \eqn{n\times Q}{n*Q} | ||
| #' matrix (multi-behavior) with times of adoption. Required when \code{graph} is not a \code{diffnet}. | ||
| #' @param t0,t1 Optional integer scalars defining the first and last observed | ||
| #' periods. If missing and \code{toa} is provided, \code{t0} defaults to 1 | ||
| #' and \code{t1} to \code{max(toa, na.rm=TRUE)}. | ||
| #' @param name Optional character scalars used only when coercing | ||
| #' inputs into a \code{diffnet} object (passed to \code{new_diffnet}). | ||
| #' @param behavior Which behaviors to include when \code{toa} is a matrix (multi-diffusion). | ||
| #' Can be \code{NULL} (all), a numeric index vector, or a character vector matching \code{colnames(toa)}. | ||
| #' @param combine Character scalar. How to combine multiple behaviors when \code{toa} is a matrix: | ||
| #' \code{"none"} (analyze each behavior separately), \code{"pooled"} (stack rows across behaviors), | ||
| #' \code{"average"} (per-actor mean of TOA across selected behaviors), or | ||
| #' \code{"earliest"} (per-actor minimum TOA). Ignored for single-behavior. | ||
| #' @param min_adopters Integer scalar. Minimum number of adopters required to compute correlations | ||
| #' for any analysis cell (default 3). | ||
| #' @param degree_strategy Character scalar. How to aggregate degree measures across | ||
| #' time periods: \code{"mean"} (default), \code{"first"}, or \code{"last"}. | ||
| #' @param bootstrap Logical scalar. Whether to compute bootstrap confidence intervals. | ||
| #' @param R Integer scalar. Number of bootstrap replicates (default 1000). | ||
| #' @param conf.level Numeric scalar. Confidence level for bootstrap intervals (default 0.95). | ||
| #' @param valued Logical scalar. Whether to use edge weights in degree calculations. | ||
| #' @param ... Additional arguments passed on when coercing to \code{diffnet}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, for roxygen2, we want to use the markdown form. Instead of using \code{abc}, we want abc, instead of using \code{\link{diffnet}} we want [diffnet()], etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add this information about the preference to .github/copilot-instructions.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated roxygen2 documentation to use markdown syntax instead of LaTeX. Changed \code{abc} to abc, \code{\link{diffnet}} to [diffnet()], and \itemize to markdown bullet lists throughout the function documentation. Also created .github/copilot-instructions.md with these documentation preferences. (48be376)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created .github/copilot-instructions.md with documentation style preferences, including the preference for markdown syntax over LaTeX in roxygen2 documentation. (48be376)
R/stats.R
Outdated
| #' \itemize{ | ||
| #' \item \code{"none"}: computes separate results per behavior (see Value). | ||
| #' \item \code{"pooled"}: stacks (actor, behavior) rows for adopters and runs a single analysis. | ||
| #' \item \code{"average"}: one row per actor using the mean TOA of adopted behaviors. | ||
| #' \item \code{"earliest"}: one row per actor using the minimum TOA of adopted behaviors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, instead of using \itemize, we can simply use markdown bullets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converted \itemize blocks to markdown bullet lists throughout the documentation as requested. (48be376)
R/stats.R
Outdated
| #' \dontrun{ | ||
| #' set.seed(999) | ||
| #' n <- 40; t <- 5; q <- 2 | ||
| #' garr <- rgraph_ws(n, t, p=.3) | ||
| #' diffnet_multi <- rdiffnet(seed.graph = garr, t = t, seed.p.adopt = rep(list(0.1), q)) | ||
| #' | ||
| #' # pooled (one combined analysis) | ||
| #' degree_adoption_diagnostic(diffnet_multi, combine = "pooled", bootstrap = FALSE) | ||
| #' | ||
| #' # per-behavior (matrix of correlations; one column per behavior) | ||
| #' degree_adoption_diagnostic(diffnet_multi, combine = "none", bootstrap = FALSE) | ||
| #' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would only use dontrun if the code takes long to run. I suspect this is not the case here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed \dontrun{} from the examples - they now execute quickly and are suitable for CRAN checks. (48be376)
R/stats.R
Outdated
| if (!is.null(x$bootstrap)) { | ||
| cat("\nBootstrap Results:\n") | ||
| cat(sprintf(" Replicates: %d\n", x$bootstrap$R)) | ||
| if (!is.null(x$bootstrap$indegree$conf_level)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To simplify the code, we can create the variable:
indegree_x <- x$bootstrap$indegreeSo we don't need to use x$boostrap everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion! I've refactored the print method to use helper functions and reduce repetitive x$bootstrap calls. The code is now cleaner and more maintainable. (48be376)
R/stats.R
Outdated
| if (!is.null(x$bootstrap$indegree$bias)) cat(sprintf(" Bias: %6.3f\n", x$bootstrap$indegree$bias)) | ||
| if (!is.null(x$bootstrap$indegree$std_error)) cat(sprintf(" Std. Error: %6.3f\n", x$bootstrap$indegree$std_error)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the is.null() checks should be done only once outside of the if (!is.null(x$boostrap$indegree$conf_int)) block. Calling is.null over and over makes the code harder to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored the print method to eliminate repetitive is.null() checks and consolidated the logic into helper functions for better readability. (48be376)
R/stats.R
Outdated
| undirected <- isTRUE(x$undirected) | ||
|
|
||
| # Helper used in both modes ------------------------------------------------ | ||
| explain <- function(label, r, ci, lvl_arg = NA_real_, thr = 0.10) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great idea. Nonetheless, the explain function seems to only work for the bootstrap function. I would rename it to something like explain_degree_test or something along those lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed the function from explain to explain_degree_correlation to better reflect its specific purpose in interpreting degree-adoption correlations. (48be376)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new function degree_adoption_diagnostic is using a lot of space on its own. Let's move it and all the related helpers and methods to a separate file under R/degree_adoption_diagnostic.R.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the entire degree_adoption_diagnostic() function, related helper functions, and print method to a new file R/degree_adoption_diagnostic.R to improve code organization and reduce the size of stats.R. (48be376)
R/stats.R
Outdated
| } | ||
|
|
||
| # Branch on combine mode --------------------------------------------------- | ||
| if (is.null(x$combine) || x$combine != "none") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ifelse block is too long. Let's write a wrapper instead that replaces the entire block. It should receive the x object explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored the print method by extracting the long conditional logic into separate helper functions (print_single_behavior_results and print_multi_behavior_results) to improve readability and maintainability. (48be376)
R/stats.R
Outdated
| ci_includes_zero <- (length(ci) >= 2) && is.finite(ci[1]) && is.finite(ci[2]) && (ci[1] <= 0 && ci[2] >= 0) | ||
| lvl_local <- if (!is.na(lvl_arg)) lvl_arg else 95 | ||
|
|
||
| if (!abs_big && !ci_includes_zero) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ifelse block is also long. I think the explain function should go in its own R file, separate from the new degree test function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the explain_degree_correlation function and all print method logic to the new R/degree_adoption_diagnostic.R file along with the main function. This separates all related functionality into its own dedicated file. (48be376)
…cumentation syntax, add NEWS entry Co-authored-by: gvegayon <[email protected]>
Added the new |
| expect_error( | ||
| degree_adoption_diagnostic(dn, conf.level = 1.5), | ||
| "'conf.level' must be between 0 and 1" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aoliveram, this test is failing (not giving an error). Also, running the function throws lots of warnings:
41: In cor(d$outdegree, d$toa, use = "complete.obs") :
the standard deviation is zero|
@gvegayon thanks for catching the errors! Let's see if it passes all the checks... |
✅ All review feedback successfully addressed
Completed changes:
.github/copilot-instructions.mdwith documentation preferencesNEWS.mdto include the newdegree_adoption_diagnostic()functiondegree_adoption_diagnostic()and related functions to separate fileR/degree_adoption_diagnostic.R\code{abc}to`abc`\code{\link{diffnet}}to`[diffnet()]`\itemizeto markdown bullet lists\dontrunfrom examples (examples now run quickly)explainfunction toexplain_degree_correlationfor claritySummary:
The
degree_adoption_diagnostic()function has been successfully refactored according to all review feedback. The code is now better organized, uses proper markdown documentation syntax, and follows the package's coding standards. All functionality has been preserved while improving maintainability and readability.✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.