Skip to content

clarify jags code#144

Open
d-morrison wants to merge 4 commits into
mainfrom
clarify-jags
Open

clarify jags code#144
d-morrison wants to merge 4 commits into
mainfrom
clarify-jags

Conversation

@d-morrison

@d-morrison d-morrison commented Jan 5, 2026

Copy link
Copy Markdown
Member

Refactors y1 calculation in JAGS code by introducing change_y intermediate variable.

Shouldn't affect numerical results, as shown by the run_mod() snapshot tests.

Copilot AI review requested due to automatic review settings January 5, 2026 07:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR improves the readability and maintainability of the JAGS model code by refactoring the calculation of y1 to use an intermediate variable and moving inline comments to separate lines for better clarity.

  • Extracted the change_y intermediate variable from the inline y1 calculation
  • Moved the explanatory comment about par[,,2] to a separate line
  • Improved code alignment for better readability

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
inst/extdata/model.jags Refactored y1 calculation by introducing change_y intermediate variable and improved comment placement
DESCRIPTION Version bump from 0.0.0.9044 to 0.0.0.9045 to reflect the code clarification

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov

codecov Bot commented Jan 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

Copilot AI review requested due to automatic review settings January 5, 2026 22:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment thread inst/extdata/model.jags
@claude

claude Bot commented May 16, 2026

Copy link
Copy Markdown
Contributor

Claude finished @d-morrison's task in 1m 28s —— View job


Code Review — clarify-jags

  • Gather context and read changed files
  • Review CLAUDE.md for repo guidelines (none found)
  • Analyze the diff and provide feedback
  • Post final review

This is a clean, focused refactoring of inst/extdata/model.jags. Here's my assessment:

What's Good

Introduction of change_y — Naming the intermediate value exp(par[subj,cur_antigen_iso,2]) as change_y is a clear improvement. It makes the y1 assignment self-documenting:

# Before:
y1[subj,cur_antigen_iso] <- y0[subj,cur_antigen_iso] + exp(par[subj,cur_antigen_iso,2])

# After:
change_y[subj,cur_antigen_iso] <- exp(par[subj,cur_antigen_iso,2])
y1[subj,cur_antigen_iso]       <- y0[subj,cur_antigen_iso] + change_y[subj,cur_antigen_iso]

The name change_y is semantically accurate (it is the change in antibody level from y₀ to y₁), and the formula for y1 now reads as plain math: y1 = y0 + change_y.

Alignment improvement — Aligning all five <- operators at the same column makes the block easier to scan visually.

JAGS semantics — Introducing deterministic nodes doesn't affect the sampling algorithm, so numerical equivalence with the snapshot tests is expected and appropriate.

Minor Observation: Comment Wording

The inline comment changed from:

# par[,,2] must be log(y1-y0)

to:

# par[,,2] is log(y1-y0):

The original "must be" subtly conveyed a parameterization constraint — that this is not an arbitrary choice but a deliberate reparameterization to ensure positivity of change_y. The new "is" is more neutral. This is a very minor point; both readings are mathematically correct. If you want to preserve that nuance, consider:

# par[,,2] is log(y1-y0), ensuring change_y > 0:

Copilot also suggested a more verbose form ("represents log(change in y from baseline (y0) to peak (y1))") but I think the current short form is fine given that change_y is now named directly below the comment.

Summary

The refactoring achieves its goal: the y1 line is now more readable without any change in model semantics. The PR is well-scoped and the snapshot test coverage backs up the claim of numerical equivalence. Ready to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants