Conversation
There was a problem hiding this comment.
Pull request overview
This PR bumps migraph to v1.5.7, updates tests to use manynet::fict_marvel, and adds new model reporting S3 methods (tidy/glance/summary) for sienaFit and a new summary.ergm() method.
Changes:
- Update
test-model_tests.Rto build the Marvel test network frommanynet::fict_marvel. - Add
tidy.sienaFit()andglance.sienaFit()S3 methods, and registersummary.sienaFit(). - Bump package version/date and add a NEWS entry for 1.5.7; update NAMESPACE registrations.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
tests/testthat/test-model_tests.R |
Switches Marvel test fixture to manynet::fict_marvel via to_uniplex() pipeline. |
R/class_models.R |
Adds tidy/glance methods for sienaFit and introduces summary.ergm()/summary.sienaFit(). |
NEWS.md |
Adds v1.5.7 release notes and date. |
NAMESPACE |
Registers S3 methods for sienaFit and summary.ergm; removes print.ergm. |
DESCRIPTION |
Bumps version to 1.5.7 and updates date. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| se = ans$se, | ||
| tstat = ans$theta/ans$se |
There was a problem hiding this comment.
tidy.sienaFit() introduces non-standard column names (se, tstat) compared with the other tidy.* methods here (std.error, statistic, p.value). This inconsistency can break downstream code that expects broom-like columns. Consider aligning the column names with the existing tidy.* outputs (keeping extra columns like dv if needed).
| se = ans$se, | |
| tstat = ans$theta/ans$se | |
| std.error = ans$se, | |
| statistic = ans$theta/ans$se |
| glance.sienaFit <- function(ans){ | ||
| tibble::tibble( | ||
| tmax = ans$tmax, | ||
| tconv.max = ans$tconv.max[,1] |
There was a problem hiding this comment.
glance.sienaFit() does not accept ..., so generics::glance() calls that pass additional arguments will error with “unused argument”. For compatibility with the generic (and with the other glance.* methods in this file), add a ... parameter (and preferably name the first argument x).
| glance.sienaFit <- function(ans){ | |
| tibble::tibble( | |
| tmax = ans$tmax, | |
| tconv.max = ans$tconv.max[,1] | |
| glance.sienaFit <- function(x, ...) { | |
| tibble::tibble( | |
| tmax = x$tmax, | |
| tconv.max = x$tconv.max[, 1] |
| #' @method tidy sienaFit | ||
| #' @export | ||
| tidy.sienaFit <- function(ans){ | ||
| tibble::tibble( | ||
| dv = ans$effects$name, | ||
| term = ans$effects$effectName, | ||
| estimate = ans$theta, | ||
| se = ans$se, | ||
| tstat = ans$theta/ans$se | ||
| ) | ||
| } |
There was a problem hiding this comment.
New S3 methods are added for tidy.sienaFit, glance.sienaFit, and summary.sienaFit, but there are no tests asserting dispatch and basic output shape/column names (contrast with tests/testthat/test-model_regression.R which tests tidy()/glance() for netlm/netlogit). Adding a small test fixture (or mocking a minimal sienaFit-like object if feasible) would help prevent regressions.
| S3method(print,network_test) | ||
| S3method(print,over_memb) | ||
| S3method(summary,diffs_model) | ||
| S3method(summary,ergm) |
There was a problem hiding this comment.
Removing S3method(print,ergm) and registering S3method(summary,ergm) is a behavioral breaking change for ergm objects and will also override ergm’s own summary.ergm() method. Given the recursion issue with tidy.ergm()/glance.ergm(), it’s safer to keep the custom printer as print.ergm() (if desired) and avoid registering summary.ergm() for the ergm class.
| S3method(summary,ergm) |
| summary.ergm <- function(x, ...){ | ||
| cat("# Fitted model results\n") | ||
| print(tidy(x)) | ||
| cat("\n# Model summary statistics\n") | ||
| print(glance(x)) | ||
| } |
There was a problem hiding this comment.
Defining a new summary.ergm() here will mask ergm’s own summary.ergm() method when migraph is loaded, and it also creates an infinite recursion: summary.ergm() calls tidy(x), but tidy.ergm() calls summary(x, ...), which will dispatch back to this summary.ergm(). This will break tidy.ergm(), glance.ergm(), and summary() for ergm fits. Consider restoring print.ergm() (as before) and not registering summary.ergm(), or implement a non-conflicting helper (or a method for a migraph-specific subclass) that does not override ergm’s summary method.
| tidy.sienaFit <- function(ans){ | ||
| tibble::tibble( | ||
| dv = ans$effects$name, | ||
| term = ans$effects$effectName, | ||
| estimate = ans$theta, | ||
| se = ans$se, | ||
| tstat = ans$theta/ans$se |
There was a problem hiding this comment.
tidy.sienaFit() does not accept ..., so generics::tidy() calls that pass additional arguments will error with “unused argument”. To stay compatible with the generics/broom style used elsewhere in this file (e.g., tidy.netlm, tidy.ergm), add a ... parameter (and preferably name the first argument x).
| tidy.sienaFit <- function(ans){ | |
| tibble::tibble( | |
| dv = ans$effects$name, | |
| term = ans$effects$effectName, | |
| estimate = ans$theta, | |
| se = ans$se, | |
| tstat = ans$theta/ans$se | |
| tidy.sienaFit <- function(x, ...) { | |
| tibble::tibble( | |
| dv = x$effects$name, | |
| term = x$effects$effectName, | |
| estimate = x$theta, | |
| se = x$se, | |
| tstat = x$theta / x$se |
Description
Checklist: