Skip to content

[WIP] Refactor allFit to avoid update() in optimizer refits#13

Draft
Copilot wants to merge 5 commits into
masterfrom
copilot/refactor-allfit-function
Draft

[WIP] Refactor allFit to avoid update() in optimizer refits#13
Copilot wants to merge 5 commits into
masterfrom
copilot/refactor-allfit-function

Conversation

Copilot AI commented Mar 18, 2026

Copy link
Copy Markdown

This PR refactors allFit() to avoid update() in the optimizer refit loop and rely on the more direct refit() path instead.

Changes made

  • Updated R/allFit.R so allFit() now refits each optimizer run via refit(object, control=ctrl) rather than update(...).
  • Removed the remaining internal update() fallback from the allFit refit path.
  • Preserved optimizer/method control handling (optCtrl, maxfun, optimizer-specific method/algorithm wiring).
  • Kept the nlmer warning behavior guarded by start_from_mle && isNLMM(object).
  • Updated the focused regression test in tests/testthat/test-allFit.R to validate robustness without relying on update()-style call$data lookup.

Validation

  • Attempted targeted test execution for test-allFit, but this environment does not provide R/Rscript, so runtime tests could not be executed locally.
  • Performed static/diff validation and automated code review/security checks in this environment.
Original prompt

This section details on the original issue you should resolve

<issue_title>refactor allFit</issue_title>
<issue_description>allFit uses update(), which is fragile and slower than it needs to be. It would be faster and more robust to only re-do the optimize(Gl|L)mer + mkMermod part of the pipeline ... More generally, update() could be optimized to be smarter about which components are being changed and only re-running the necessary bits: i.e., generalizing from refit() (only response variable changes) to use cases like

  • fixed effects change, structure of random effects doesn't (can substitute new X and leave everything else alone)
    </issue_description>

<agent_instructions>Basically, try to find a way to make the allFit function faster.</agent_instructions>

Comments on the Issue (you are @copilot in this section)


📍 Connect Copilot coding agent with Jira, Azure Boards or Linear to delegate work to Copilot in one click without leaving your project management tool.

@annahuynhly

Copy link
Copy Markdown
Owner

@copilot could you please try again?

Copilot AI and others added 2 commits March 22, 2026 22:02
Co-authored-by: annahuynhly <97189987+annahuynhly@users.noreply.github.com>
Agent-Logs-Url: https://github.com/annahuynhly/lme4/sessions/30bb2320-1032-4bfe-83c2-4a0189123f9c

Copilot AI commented Mar 22, 2026

Copy link
Copy Markdown
Author

@copilot could you please try again?

Implemented a new attempt and pushed it in 70d31d6: allFit(start_from_mle=TRUE) now takes a faster/less-fragile path via refit(object, control=ctrl) (with NLMM fallback preserving prior update(..., start=...) behavior), while start_from_mle=FALSE keeps existing update behavior. I also added a focused regression test for start_from_mle data-lookup robustness.

@annahuynhly annahuynhly left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please really focus on this key comment:

It would be faster and more robust to only re-do the optimize(Gl|L)mer + mkMermod part of the pipeline ... More generally, update() could be optimized to be smarter about which components are being changed and only re-running the necessary bits:

Comment thread R/allFit.R Outdated
assign("pars", start, envir = tmp_env, inherits = FALSE)
}
on.exit({ environment(form) <- env }, add = TRUE)
if (is.null(start)) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot don't you think that it's ironic you used update here, but the main point is to NOT use update...?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in b0e5dae (plus e950e49): I removed the remaining update() usage from allFit() and switched the refit path to refit(object, control=ctrl) for optimizer runs. No UI changes here, so there is no UI screenshot.

Copilot AI and others added 2 commits March 23, 2026 03:03
Co-authored-by: annahuynhly <97189987+annahuynhly@users.noreply.github.com>
Agent-Logs-Url: https://github.com/annahuynhly/lme4/sessions/895f4c08-9385-4848-bffd-3b521aaec1cb
Co-authored-by: annahuynhly <97189987+annahuynhly@users.noreply.github.com>
Agent-Logs-Url: https://github.com/annahuynhly/lme4/sessions/895f4c08-9385-4848-bffd-3b521aaec1cb

Copilot AI commented Mar 23, 2026

Copy link
Copy Markdown
Author

@copilot please really focus on this key comment:

It would be faster and more robust to only re-do the optimize(Gl|L)mer + mkMermod part of the pipeline ... More generally, update() could be op...

Done in b0e5dae (with follow-up e950e49): allFit() now avoids update() in its refit loop and uses refit(object, control=ctrl) so it re-runs optimizer/model rebuild without the full update pipeline. I also updated the robustness test accordingly. No UI changes in this PR, so there is no UI screenshot.

Copilot AI changed the title [WIP] Refactor allFit for improved performance [WIP] Refactor allFit to avoid update() in optimizer refits Mar 23, 2026
Copilot AI requested a review from annahuynhly March 23, 2026 03:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor allFit

2 participants