refactor: extract shared helpers to deduplicate SG/MG estimation logic#11
Merged
Wenchao-Ma merged 11 commits intoMar 27, 2026
Merged
Conversation
Create R/estimation_utils.R with shared functions: - init_control(): Unified control parameter setup with is_mg flag - init_solver_args(): Solver argument initialization - init_solver(): Solver vector expansion Refactor SingleGroup_Estimation.R and MultipleGroup_Estimation.R to use these helpers, reducing ~70 lines of duplicated code. No behavioral changes - verified with smoke tests.
Add to estimation_utils.R: - check_em_convergence(): Unified convergence checking with configurable criteria - print_em_progress(): Standardized iteration output formatting Refactor both SingleGroup_Estimation.R and MultipleGroup_Estimation.R EM loops to use these helpers, reducing ~25 lines of duplicated convergence code. No behavioral changes - verified with smoke tests including verbose output.
itemparm.GDINA was duplicating 75 lines of logic already in coef.GDINA. Now it simply issues the deprecation warning and delegates to coef(), reducing maintenance burden and ensuring consistency. The function signature and deprecation warning remain unchanged for backward compatibility.
Add to estimation_utils.R: - init_constraint_matrices(): Creates constraint matrices for monotonicity - init_design_matrices(): Creates design matrices based on model type Refactor both estimation files to use these helpers, eliminating ~45 lines of duplicated code for setting up optimization constraints. Verified with single and multiple group tests with monotonicity constraints.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors shared single-group (SG) and multiple-group (MG) estimation setup/loop logic into a new internal helper module to reduce duplication, and simplifies itemparm.GDINA() to delegate to coef().
Changes:
- Added
R/estimation_utils.Rwith shared internal helpers for control/solver initialization, constraints/design matrices, and EM convergence/progress reporting. - Refactored
SG.EstandMG.Estto call shared helpers instead of maintaining duplicated blocks. - Refactored
itemparm.GDINA()to delegate tocoef()(but the current file state has a structural issue).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
R/estimation_utils.R |
New shared helper functions used by SG/MG estimation to centralize duplicated logic. |
R/SingleGroup_Estimation.R |
Replaced duplicated control/solver/constraint/design/convergence code with helper calls. |
R/MultipleGroup_Estimation.R |
Same refactor as SG: uses shared helpers for initialization and EM progress/convergence. |
R/itemparm.GDINA.R |
Intended to delegate to coef(), but currently contains leftover code after the function closing brace. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The previous refactor left orphaned code after the function closing brace. This commit properly removes all the old duplicated logic that should have been deleted when consolidating to delegate to coef().
When maxitr has invalid length (neither 1 nor ncat), the original code only emitted a warning but left control unset. This caused downstream errors when code tried to use control. Changed to stop() for consistent validation behavior with lower.p and upper.p parameters, and to fail fast with a clear error message.
The new estimation_utils.R file was missing from the Collate field in DESCRIPTION, causing R CMD build to fail with 'unable to collate and parse R files' error. Added 'estimation_utils.R' to Collate before SingleGroup_Estimation.R since it provides utility functions used by both estimation files.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| withSE = FALSE, SE.type = 2,digits = 4, ...){ | ||
| stopifnot(isa(object,"GDINA")) | ||
| .Deprecated("coef", package="GDINA",msg = "'itemparm' is deprecated - use 'coef' instead.") | ||
| # Delegate to coef.GDINA after mapping deprecated parameter names |
There was a problem hiding this comment.
This comment says the function maps deprecated parameter names, but the implementation forwards what unchanged. Either remove the comment or actually implement the mapping if there are legacy aliases that coef() does not accept.
Suggested change
| # Delegate to coef.GDINA after mapping deprecated parameter names | |
| # Delegate to coef.GDINA |
Comment on lines
+49
to
+54
| # Handle maxitr expansion | ||
| if (length(control$maxitr) == 1L) { | ||
| control$vmaxitr <- rep(control$maxitr, ncat) | ||
| } else if (length(control$maxitr) != ncat) { | ||
| stop("maxitr must have length of 1 or number of nonzero categories", call. = FALSE) | ||
| } else { |
There was a problem hiding this comment.
This changes behavior vs the pre-refactor SG/MG code: when control$maxitr has a length other than 1 or ncat, it now throws an error instead of warning. If this stricter validation is intended, the PR description (“No user-facing changes”) should be updated; otherwise consider preserving the prior warning behavior (while still ensuring vmaxitr is defined).
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Owner
|
Thank you! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes:
Impact:
Test Status: