Skip to content

Add HERS dataset linear regression example with interactions, interspersed with birthweight analysis (Vittinghoff Ch. 4)#381

Open
Copilot wants to merge 10 commits into
mainfrom
copilot/add-example-model-hers-dataset
Open

Add HERS dataset linear regression example with interactions, interspersed with birthweight analysis (Vittinghoff Ch. 4)#381
Copilot wants to merge 10 commits into
mainfrom
copilot/add-example-model-hers-dataset

Conversation

Copilot AI commented Apr 7, 2026

Copy link
Copy Markdown
Contributor
  • Add HERS dataset linear regression example with interactions, interspersed with birthweight analysis
  • Interaction model: LDL ~ age + BMI + age:BMI (predictors are age and BMI; statin use shown as color grouping)
  • All HERS plots faceted by statins with label_both
  • HERS content in subfiles; GOF/diagnostics/model-selection/stratified-regression sections included
  • GGally added to DESCRIPTION Suggests and renv.lock (v2.4.0)
  • regress3d (from d-morrison/regress3d@add-show_legend) added to DESCRIPTION Suggests and renv.lock
  • Merge conflicts resolved (WORDLIST deduplicated+sorted; renv.lock restored to 298-package base)
  • Fix CI failure: GGally::wrap conflicted with pander::wrap — now using GGally::wrap() explicitly in fig-hers-key-vars chunk
  • Add note in data notation that HT is not a predictor (baseline LDL); add age ($A$) to variable notation
  • Replace fig-hers-scatter (2D) with an interactive 3D plotly scatter (age × BMI × LDL, colored by statin use)
  • Add 3D regression plane to parallel-lines model section using regress3d::add_3d_surface() for LDL ~ age + BMI
  • Add 3D regression surface to interaction model section using regress3d::add_3d_surface() for LDL ~ age + BMI + age:BMI
  • Add HERS stratified regression section (_sec_hers_lm_stratified.qmd) to match birthweight stratified section
  • Fix CI: renv.lock restored from 626ab42 (297 pkgs) + regress3d = 298 packages total; conflicted, GGally, ggstats, patchwork all restored
  • Remove multi-column div layout from 3D figures in _sec_hers_lm_parallel.qmd and _sec_hers_lm_interact.qmd
  • Stratify 3D regression surface figures by statin use using panel-tabset subpanels (Statins: No / Statins: Yes), with the same model surface shown over the full data range for each group

Copilot AI linked an issue Apr 7, 2026 that may be closed by this pull request
Copilot AI changed the title [WIP] Add an example model with the HERS dataset for linear regression Add HERS dataset linear regression example (Vittinghoff Ch. 4) Apr 7, 2026
Copilot AI requested a review from d-morrison April 7, 2026 20:08
@d-morrison

Copy link
Copy Markdown
Owner

add a model with interactions, like we did for the birthweight data. intersperse the HERS analysis with the corresponding sections of the birthweight analysis

@d-morrison

Copy link
Copy Markdown
Owner

@copilot add a model with interactions, like we did for the birthweight data. intersperse the HERS analysis with the corresponding sections of the birthweight analysis

Copilot AI commented Apr 7, 2026

Copy link
Copy Markdown
Contributor Author

@copilot add a model with interactions, like we did for the birthweight data. intersperse the HERS analysis with the corresponding sections of the birthweight analysis

Done in commit 27418f9. The HERS analysis is now interspersed with the birthweight sections:

  • 2.2.4 Motivating example: hers data — data loading, scatter plot, and notation (H = HT treatment, B = BMI)
  • 2.2.5 Parallel lines regression for hers data — LDL ~ HT + BMI, mirroring the birthweight parallel lines model
  • 2.2.7 Interactions in hers data — LDL ~ HT + BMI + HT:BMI, added right after the birthweight interaction section (2.2.6)

Each HERS section follows the same structure as its birthweight counterpart: model equation, parameter table, and fitted-values plot.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • glmnet.stanford.edu
    • Triggering command: /opt/R/4.5.3/lib/R/bin/exec/R /opt/R/4.5.3/lib/R/bin/exec/R --no-echo --no-restore --file=/opt/quarto/share/rmd/rmd.R (dns block)
    • Triggering command: /opt/R/4.5.3/lib/R/bin/exec/R /opt/R/4.5.3/lib/R/bin/exec/R --no-echo --no-restore --file=/opt/quarto/share/rmd/rmd.R unam�� (dns block)
  • olsrr.rsquaredacademy.com
    • Triggering command: /opt/R/4.5.3/lib/R/bin/exec/R /opt/R/4.5.3/lib/R/bin/exec/R --no-echo --no-restore --file=/opt/quarto/share/rmd/rmd.R (dns block)
    • Triggering command: /opt/R/4.5.3/lib/R/bin/exec/R /opt/R/4.5.3/lib/R/bin/exec/R --no-echo --no-restore --file=/opt/quarto/share/rmd/rmd.R unam�� (dns block)
  • packagemanager.posit.co
    • Triggering command: /usr/bin/curl /usr/bin/curl --config /tmp/Rtmpz21USI/renv-download-config-26a159b3cb76 (dns block)
    • Triggering command: /usr/bin/curl /usr/bin/curl --config /tmp/Rtmpz21USI/renv-download-config-26a143be690e (dns block)
    • Triggering command: /usr/bin/curl /usr/bin/curl --config /tmp/Rtmpz21USI/renv-download-config-26a1777d9204 (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI changed the title Add HERS dataset linear regression example (Vittinghoff Ch. 4) Add HERS dataset linear regression example with interactions, interspersed with birthweight analysis (Vittinghoff Ch. 4) Apr 7, 2026
@d-morrison

Copy link
Copy Markdown
Owner

@copilot add interaction between bmi and statin use to the HERS model, and remove the interaction between bmi and HT

Copilot AI commented Apr 9, 2026

Copy link
Copy Markdown
Contributor Author

@copilot add interaction between bmi and statin use to the HERS model, and remove the interaction between bmi and HT

Done in commit f7d07a6. The interaction model is now LDL ~ HT + BMI + statins + BMI:statins:

  • Removed HT:BMI interaction
  • Added statins as a main effect and BMI:statins as the interaction term
  • Added statin use ($U$/$V$) to the data notation section
  • Updated the scatter plot to colour/shape by statin use (no vs yes)

The model equation is now:
$$\mu(h,v,b) = \beta_0 + \beta_H h + \beta_V v + \beta_B b + \beta_{VB}(v \cdot b)$$

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • rspm-sync.rstudio.com
    • Triggering command: /usr/bin/curl /usr/bin/curl --config /tmp/RtmpdqGxja/renv-download-config-2677336aef2b (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@d-morrison

Copy link
Copy Markdown
Owner

@copilot resolve merge conflicts

@d-morrison d-morrison removed their request for review April 10, 2026 18:54

Copilot AI commented Apr 10, 2026

Copy link
Copy Markdown
Contributor Author

@copilot resolve merge conflicts

Done in commit 4ae72cf. The key conflict was in Linear-models-overview.qmd, where main had refactored the "Understanding" section into a new subfile _sec_linreg_understanding.qmd. The HERS includes were moved there at the correct positions (after birthweight parallel lines and after birthweight interactions), and the WORDLIST was merged with both sets of words deduplicated and sorted.

Copilot AI requested a review from d-morrison April 10, 2026 19:05
Copilot AI review requested due to automatic review settings April 14, 2026 18:15

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 integrates a HERS dataset linear regression example into the “Linear Models Overview” material, interleaving it with the existing birthweight discussion to mirror the narrative flow of Vittinghoff Ch. 4.

Changes:

  • Adds two new HERS section subfiles: one for a parallel-lines model and one for an interaction model (BMI × statin use).
  • Inserts those HERS sections into _sec_linreg_understanding.qmd at the intended points in the birthweight narrative.
  • Updates inst/WORDLIST (but currently with apparent duplication/ordering issues).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
inst/WORDLIST Adds spellcheck allowlist terms, but the resulting file contains duplicates/merge artifacts that should be cleaned up.
_sec_linreg_understanding.qmd Includes the new HERS sub-sections into the main linear regression “understanding” chapter flow.
_sec_hers_lm_parallel.qmd New HERS intro + parallel lines regression section (treatment indicator + BMI).
_sec_hers_lm_interact.qmd New HERS interaction section (adds statins and BMI:statins interaction; plot by statin use).
_sec_exm_hers_lm.qmd Adds a standalone HERS example section that appears unused/unreferenced and conflicts with the PR’s stated restructuring goals.

Comment thread _sec_exm_hers_lm.qmd Outdated
Comment thread _sec_exm_hers_lm.qmd Outdated
Comment thread _sec_exm_hers_lm.qmd Outdated
Comment thread inst/WORDLIST
@d-morrison

Copy link
Copy Markdown
Owner

@copilot apply changes based on the comments in this thread

Copilot AI commented Apr 14, 2026

Copy link
Copy Markdown
Contributor Author

@copilot apply changes based on the comments in this thread

Done in commit 8303639:

  • Deleted _sec_exm_hers_lm.qmd — the file was not referenced by any {{< include >}} directive or rendered page, and the standalone HERS section was already superseded by the interspersed _sec_hers_lm_parallel.qmd and _sec_hers_lm_interact.qmd subfiles.
  • Fixed inst/WORDLIST — reduced from 85 entries (21 duplicates) to 64 unique entries, sorted alphabetically.

- Per CLAUDE.md "Subfiles must NOT begin with a section heading",
  move the 7 leading ### headings from
  _subfiles/Linear-models-overview/_sec_hers_*.qmd
  into the parent files (_sec_linreg_understanding.qmd and
  _sec_linreg_assess.qmd), where the table-of-contents tree
  controls them and where Quarto's chapter-level rendering picks
  them up consistently.
- _sec_hers_lm_model_selection.qmd: replace `library(lmtest)` +
  `lrtest(...)` with namespace-qualified `lmtest::lrtest(...)`.
  Avoids surprising the reader (and downstream chunks) with a
  full attach that may shadow generics.
@claude

claude Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @d-morrison's task in 3m 31s —— View job


Code Review — PR #381

  • Gather context and read changed files
  • Review all 7 HERS subfiles
  • Review parent chapter includes (_sec_linreg_understanding.qmd, _sec_linreg_assess.qmd)
  • Review WORDLIST, DESCRIPTION, renv.lock
  • Post findings

What the most recent commit (92b54ff) fixed

  • All 7 HERS subfiles no longer start with ### headings — they were moved to parent files per CLAUDE.md.
  • lmtest::lrtest() is now namespace-qualified in _sec_hers_lm_model_selection.qmd.

Remaining issues

1. ### heading for lm2 diagnostics is still inside the subfile (_sec_hers_lm_diagnostics.qmd:51)

### Diagnostics for `hers` interaction model

The parent _sec_linreg_assess.qmd correctly provides the ### Diagnostics for 'hers' parallel-planes model heading in the parent, but the corresponding heading for the interaction model is still inside the subfile. This creates an inconsistency: one section heading is in the parent, the other is buried in the subfile and won't appear in the chapter outline.

Fix: move ### Diagnostics for \hers` interaction modelfrom line 51 of_sec_hers_lm_diagnostics.qmdto_sec_linreg_assess.qmd(just before the{{< include >}}` for the diagnostics subfile, or split the subfile into two). The parent already has the pattern — just add the second heading there.

Fix this →


2. GOF tables use #| label: chunk option instead of div format (_sec_hers_lm_gof.qmd:11–16, 25–30)

```{r}
#| label: tbl-hers-aic-bic
#| code-fold: true
AIC(hers_lm1, hers_lm2)
BIC(hers_lm1, hers_lm2)

CLAUDE.md requires `:::{#tbl-...}` div format (with caption) for cross-referenceable tables, not the `label` chunk option. Same issue for `tbl-hers-deviance` at lines 25–30. Neither block has a caption.

Fix: wrap both chunks in `:::{#tbl-hers-aic-bic}` / `:::{#tbl-hers-deviance}` divs with a caption line.

[Fix this →](https://claude.ai/code?q=In%20_subfiles%2FLinear-models-overview%2F_sec_hers_lm_gof.qmd%20on%20branch%20copilot%2Fadd-example-model-hers-dataset%20of%20d-morrison%2Frme%3A%20Convert%20both%20code%20chunks%20(tbl-hers-aic-bic%20at%20lines%2011-16%2C%20tbl-hers-deviance%20at%20lines%2025-30)%20from%20%23%7C%20label%3A%20chunk%20option%20to%20%3A%3A%3A%7B%23tbl-hers-aic-bic%7D%20and%20%3A%3A%3A%7B%23tbl-hers-deviance%7D%20div%20format%20with%20caption%20lines%2C%20per%20CLAUDE.md%20convention.)

---

#### 3. Missing `#| code-fold: true` in stratified regression chunks (`_sec_hers_lm_stratified.qmd:22, 50`)

Both table chunks inside `:::{#tbl-hers-lm-strat}` (line 22) and `:::{#tbl-hers-lm-statin-interact}` (line 50) lack `#| code-fold: true`.

[Fix this →](https://claude.ai/code?q=In%20_subfiles%2FLinear-models-overview%2F_sec_hers_lm_stratified.qmd%20on%20branch%20copilot%2Fadd-example-model-hers-dataset%20of%20d-morrison%2Frme%3A%20Add%20%23%7C%20code-fold%3A%20true%20to%20the%20code%20chunk%20inside%20the%20tbl-hers-lm-strat%20div%20(line%2022)%20and%20to%20the%20code%20chunk%20inside%20the%20tbl-hers-lm-statin-interact%20div%20(line%2050).)

---

#### 4. Wrong label prefix in model selection (`_sec_hers_lm_model_selection.qmd:11`)

```qmd
#| label: lst-hers-lrt

The lst- prefix is for Quarto code listings, not result outputs. If this output is meant to be cross-referenced as a result, use tbl- (or no label if not cross-referenced). Also, the chunk is not wrapped in a div with a caption.

Fix this →


5. Notation mismatch in stratified model equation (_sec_hers_lm_stratified.qmd:10–12)

$$
\E{LDL|B=b, U=u} =
\beta_{no} (1-v) + \beta_{no,B}\, b(1-v) +
\beta_{yes} v + \beta_{yes,B}\, bv
$$ {#eq-hers-strat}

The left side conditions on $U = u$ (the categorical variable), but the right side uses $v$ (the indicator for $V = $ "yes") without ever establishing the link. Fix: change the left side to condition on $V = v$ so it's consistent with the right-hand side and with the data notation section which defines $V$ as the indicator.

Fix this →


6. age silently dropped in stratified model (_sec_hers_lm_stratified.qmd:6–7)

The notes say "as an alternative to the age–BMI models above", but the stratified model uses only BMI (LDL ~ statins + statins:BMI - 1), silently dropping age. Since hers_lm1 and hers_lm2 both include age, a reader will notice the omission without explanation. One sentence clarifying this (e.g., "For simplicity, this stratified model focuses on the BMI–statin relationship and omits age") would resolve the confusion.


7. Redundant hers_plot_data recomputation (_sec_hers_lm_interact.qmd:37–38)

hers_plot_data <- hers |>
  dplyr::filter(!is.na(age), !is.na(BMI), !is.na(LDL))

This is identical to the same assignment in _sec_hers_lm_parallel.qmd:28–30. Since both subfiles are included in the same document, the variable already exists by the time the interact section renders. The redundant assignment is harmless but noisy.


8. regress3d pinned to branch, not a commit SHA (DESCRIPTION:85)

d-morrison/regress3d@add-show_legend

A branch ref is mutable — force pushes or branch deletion silently break reproducibility. Prefer a specific commit SHA or tag once the work stabilizes:

d-morrison/regress3d@<SHA>

Positive notes

  • Last commit correctly hoisted all 7 leading ### headings to parent files — the subfile-heading convention is now satisfied for all top-level sections.
  • lmtest::lrtest() is namespace-qualified — no library() call polluting the session.
  • GGally::wrap() correctly namespaced to avoid pander::wrap conflict.
  • Explicit relevel(ref = ...) for both HT ("placebo") and statins ("no") is clear and robust.
  • Panel-tabset (Statins: No / Statins: Yes) with a single model surface per tab is a clean stratification pattern.
  • WORDLIST is sorted with no duplicates; new entries (Statins, sciid, drinkany, bv, drk, nw, smk, tabset) are all justified.
  • code-fold: true applied consistently across data, parallel, interact, and diagnostics subfiles.

| Branch

claude added 2 commits June 19, 2026 08:13
1. Hoist the lm2-diagnostics heading to the parent: split
   _sec_hers_lm_diagnostics.qmd into lm1 and a new lm2 subfile, with the
   '### Diagnostics for hers interaction model' heading now in
   _sec_linreg_assess.qmd (matching the lm1 heading)
2. Convert the AIC/BIC and deviance GOF outputs to :::{#tbl-...} div
   format with captions (instead of #| label: chunk options)
4. Fix the wrong label prefix: lst-hers-lrt -> a {#tbl-hers-lrt} div
3. Add #| code-fold: true to the stratified-regression table chunks
5. Fix the stratified-model notation: condition on V=v (the 0/1 statin
   indicator defined in the data-notation section) rather than U=u, so
   the indicator arithmetic (1-v), bv is well-defined
6. Explain that age is intentionally omitted from the stratified model
7. Drop the redundant hers_plot_data recomputation in the interaction
   subfile (it is already computed in the parallel-planes subfile, which
   renders earlier)
8. Pin the regress3d Remotes entry to commit 158d63c (from renv.lock)
   instead of the mutable add-show_legend branch ref

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011CqGbHzLqKVGnbEpLEdadG
@claude

claude Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Claude finished @d-morrison's task in 5m 11s —— View job


Code Review — PR #381 (re-review after 89405d3)

  • Gather context and read changed files
  • Verify all 8 previously flagged issues are resolved
  • Check for new issues
  • Post findings

All 8 previous findings resolved ✓

# Finding Status
1 lm2-diagnostics ### heading inside subfile ✅ Fixed — split into _sec_hers_lm_diagnostics_lm2.qmd; heading now in _sec_linreg_assess.qmd
2 GOF tables using #| label: chunk option instead of div format ✅ Fixed — both are now :::{#tbl-hers-aic-bic} / :::{#tbl-hers-deviance} divs
3 Missing #| code-fold: true in stratified chunks ✅ Fixed — both tbl-hers-lm-strat and tbl-hers-lm-statin-interact chunks now have it
4 Wrong lst- label prefix in model selection ✅ Fixed — now :::{#tbl-hers-lrt} div with caption
5 Notation mismatch U=u vs v in stratified equation ✅ Fixed — LHS now \E{LDL|B=b, V=v}
6 age silently dropped in stratified model ✅ Fixed — explanation added in ::: notes block
7 Redundant hers_plot_data recomputation in interact file ✅ Fixed — interact file now uses the object defined in the parallel file
8 regress3d pinned to mutable branch ref ✅ Fixed — DESCRIPTION now pins to 158d63cee6d823eccb5b0fc45e11f013ea0097d7

Remaining issues (new)

1. tbl-hers-lm2 caption lacks equation cross-reference (_sec_hers_lm_interact.qmd:45)

The parallel table caption reads:

Regression parameter estimates for [Model @eq-hers-parallel]

The interaction table caption reads:

HERS interaction model

For consistency, change the interaction caption to reference the equation number:

Regression parameter estimates for [Model @eq-hers-interact]

Fix this →


2. Redundant library() calls in parallel file (_sec_hers_lm_parallel.qmd:50–51)

library(plotly)
library(regress3d)

All downstream calls in this file already use plotly:: and regress3d:: prefixes, so these library() calls are redundant. Remove them; the interact file works the same way (no library() calls at all), confirming they are not needed.

Fix this →


3. hers_no / hers_yes re-defined to identical values in interact file (_sec_hers_lm_interact.qmd:57, 93)

The parallel file (lines 52, 88) defines:

hers_no <- hers_plot_data |> dplyr::filter(statins == "no")
hers_yes <- hers_plot_data |> dplyr::filter(statins == "yes")

The interact file (lines 57, 93) re-defines them to the same values. Since both subfiles render sequentially in the same document, the objects already exist. Remove the re-definitions from the interact file; the parallel file's definitions are sufficient.

Fix this →


4. Forward reference to stratified model (_sec_hers_lm_parallel.qmd:123)

The stratified regression model below addresses this...

CLAUDE.md: "Introduce concepts before using them — no forward references."

The stratified section hasn't been introduced yet when this sentence appears. Rephrase to remove the forward reference, e.g.:

"Statin use appears to shift the baseline LDL level, a pattern the parallel-planes model cannot capture because it does not include statins as a predictor."

Fix this →


5. Two printed outputs inside one :::{#tbl-hers-aic-bic} div (_sec_hers_lm_gof.qmd:11–20)

AIC(hers_lm1, hers_lm2)
BIC(hers_lm1, hers_lm2)

These two calls each produce a separate printed table. Putting both inside a single :::{#tbl-hers-aic-bic} div results in two outputs under one cross-reference anchor, which Quarto may render correctly for some formats but not others. Combine them into a single table (e.g., left_join(AIC(...), BIC(...), by = "df")) or give each its own div with a separate cross-reference anchor.

Fix this →


Positive notes

  • All 8 prior findings addressed cleanly.
  • regress3d correctly pinned to a commit SHA in DESCRIPTION.
  • Diagnostics split into two subfiles with headings in the parent — clean structure.
  • GOF divs have descriptive captions and code-fold: true throughout.
  • Stratified equation LHS now matches the RHS notation (V=v).
  • lmtest::lrtest() namespace-qualified, wrapped in a proper :::{#tbl-hers-lrt} div.
  • WORDLIST sorted, no duplicates; all 9 new entries are justified.

| Branch

Copy link
Copy Markdown
Owner

Addressed all 8 findings from the latest review (commit 89405d3, main merged in cleanly — no renv.lock conflict):

  1. Address — hoisted the lm2-diagnostics ### heading to the parent. Split _sec_hers_lm_diagnostics.qmd into an lm1 file and a new _sec_hers_lm_diagnostics_lm2.qmd; the "Diagnostics for hers interaction model" heading now lives in _sec_linreg_assess.qmd, mirroring the lm1 heading.
  2. Address — converted the AIC/BIC and deviance GOF outputs to :::{#tbl-…} div format with captions (dropping the #| label: chunk options). Render confirms Table 3 (AIC/BIC) and Table 4 (deviance) now carry captions/numbers.
  3. Address — fixed the wrong lst- (listing) prefix: lst-hers-lrt → a {#tbl-hers-lrt} div (Table 5). Verified no cross-references pointed at the old label.
  4. Address — added #| code-fold: true to both stratified-regression table chunks.
  5. Address — notation fix. The data-notation section (_sec_hers_data.qmd) defines V as the 0/1 indicator for statin use U, so the RHS arithmetic (1-v), bv needs the indicator. Conditioned the LHS on V=v (rather than U=u); this keeps v ∈ {0,1} well-defined throughout.
  6. Address — added a note that age is intentionally omitted from the stratified specification (to focus on the BMI–LDL relationship by statin group).
  7. Address — removed the redundant hers_plot_data recomputation in _sec_hers_lm_interact.qmd; it's already computed in _sec_hers_lm_parallel.qmd, which renders earlier in the chapter.
  8. Address — pinned the regress3d Remotes entry to commit 158d63cee6d823eccb5b0fc45e11f013ea0097d7 (the RemoteSha already recorded in renv.lock) instead of the mutable @add-show_legend branch ref.

Checks: lintr::lint() ✓ on all changed files; spelling::spell_check_package() ✓; the HERS section renders cleanly with all tables/figures cross-referencing correctly (verified via an isolated wrapper loading hers from inst/extdata — the full Linear-models-overview render needs the rmb package used by sibling subfiles, which isn't available in this session, so the CI build job is the full-chapter gate).

@claude please re-review.


Generated by Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

👀 Picked up by workflow run #27814393620. R/Quarto/renv setup runs first (~3-5 min); Claude itself responds after that.

…F tables

- interaction-model table caption now references [Model @eq-hers-interact]
  (matching the parallel-planes table convention)
- remove redundant library(plotly)/library(regress3d) from the parallel
  subfile (all calls are namespace-qualified)
- remove redundant hers_no/hers_yes redefinitions from the interaction
  subfile (already defined in the parallel subfile, which renders earlier)
- rephrase the forward reference to the stratified model in the parallel
  subfile into a description of what the parallel-planes model cannot capture
- combine the AIC/BIC and deviance outputs into single knitr::kable tables
  so each tbl- div has one output under its cross-reference anchor
- replace 'below' with an @tbl-hers-lrt cross-reference in the GOF notes

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011CqGbHzLqKVGnbEpLEdadG

Copy link
Copy Markdown
Owner

Round 2 — addressed all 5 new findings (commit d97ef94):

  1. Address_sec_hers_lm_interact.qmd table caption: "HERS interaction model" → "Regression parameter estimates for [Model @eq-hers-interact]", matching the parallel-planes table. Render confirms Table 2 now carries the equation reference.
  2. Address — removed the redundant library(plotly) / library(regress3d) from _sec_hers_lm_parallel.qmd; every call in the file is namespace-qualified (plotly::, regress3d::).
  3. Address — removed the redundant hers_no / hers_yes redefinitions from _sec_hers_lm_interact.qmd; they're already defined in _sec_hers_lm_parallel.qmd, which renders earlier (same pattern as the prior hers_plot_data fix).
  4. Address — rephrased the forward reference in _sec_hers_lm_parallel.qmd: "The stratified regression model below addresses this…" → "…a shift the parallel-planes model cannot capture, because it does not include statin use as a predictor."
  5. Address — combined the two-output GOF tables into single knitr::kable() tables so each {#tbl-…} div has one output under its anchor. Applied to both tbl-hers-aic-bic (df/AIC/BIC in one table) and tbl-hers-deviance (the deviance table had the same two-output defect I'd introduced).

Proactive (same no-forward-reference rule): replaced "The likelihood ratio test below…" in the GOF notes with an @tbl-hers-lrt cross-reference, per CLAUDE.md ("Never use 'above'/'below' — cross-reference with @label").

Checks: lintr::lint() ✓ on all three files; spelling::spell_check_package() ✓; isolated HERS-section render ✓ — Tables 1–7 all caption/number correctly, @eq-hers-interact and @tbl-hers-lrt resolve, no broken cross-references.

@claude please re-review.


Generated by Claude Code

@claude

claude Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Claude finished @d-morrison's task in 3m 7s —— View job


Code Review — PR #381 (re-review after d97ef94)

  • Gather context
  • Verify all 5 previously flagged issues are resolved
  • Check for new issues
  • Post findings

All 5 previous findings resolved ✓

# Finding Status
1 tbl-hers-lm2 caption lacked equation cross-reference ✅ Fixed — caption now reads "Regression parameter estimates for [Model @eq-hers-interact]"
2 Redundant library(plotly) / library(regress3d) in parallel file ✅ Fixed — removed; all calls use plotly:: / regress3d:: prefixes
3 hers_no / hers_yes redefined identically in interact file ✅ Fixed — removed from interact file; objects defined once in parallel file
4 Forward reference "The stratified regression model below…" ✅ Fixed — rephrased to "a shift the parallel-planes model cannot capture, because it does not include statin use as a predictor"
5 Two printed outputs inside one :::{#tbl-hers-aic-bic} div ✅ Fixed — combined into a single data.frame() + knitr::kable() call; deviance table also converted

Remaining issues (new)

1. Redundant library(plotly) in _sec_hers_data.qmd:58

library(plotly)

Every plotly call in this chunk (plotly::plot_ly(), plotly::layout()) already uses explicit plotly:: prefix — the same fix applied to the parallel file in the last round. Remove library(plotly).

Fix this →


2. tbl-hers-lm-statin-interact caption is vague and references no equation (_sec_hers_lm_stratified.qmd:68)

Statin interaction model estimates

The left-hand table (tbl-hers-lm-strat) references @eq-hers-strat. The right-hand table shows hers_lm_statins_interact = LDL ~ statins + BMI + statins:BMI, but this model is never written out as a displayed equation — the reader must infer the specification from the R code. In the birthweight counterpart (_sec_linreg_understanding.qmd), the right-hand table references @eq-BW-lm-interact, which was introduced earlier. For HERS, the analogous equation (LDL ~ statins + BMI + statins:BMI) should be displayed before the table and the caption updated to reference it.

Fix this →


Positive notes

  • All 5 prior findings addressed cleanly and correctly.
  • @tbl-hers-lrt cross-reference in the GOF notes correctly replaces "below" — follows CLAUDE.md convention.
  • AIC/BIC and deviance tables each produce a single output under their div anchors.
  • No new library() redundancies introduced in the interact or model-selection files.
  • regress3d correctly pinned to commit SHA in DESCRIPTION.

| Branch

…t library(plotly)

- add a displayed equation (eq-hers-strat-interact) for the reference-coded
  interaction parameterization LDL ~ statins + BMI + statins:BMI, and update
  the right-hand stratified table caption to reference it (matching the
  left-hand table's reference to eq-hers-strat)
- remove redundant library(plotly) from _sec_hers_data.qmd; all plotly calls
  in that chunk are namespace-qualified (library(GGally) stays, as ggpairs()
  is called unqualified)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011CqGbHzLqKVGnbEpLEdadG

Copy link
Copy Markdown
Owner

Round 3 — addressed both new findings (commit 380c7b4):

  1. Address — removed the redundant library(plotly) from _sec_hers_data.qmd:58; all plotly calls in that chunk are plotly::-qualified. (Left library(GGally) in place — ggpairs() is called unqualified there, so it's genuinely needed.)
  2. Address — the right-hand stratified table (tbl-hers-lm-statin-interact, LDL ~ statins + BMI + statins:BMI) now has a displayed equation. Added @eq-hers-strat-interact (the reference/indicator-coded parameterization \beta_0 + \beta_V v + \beta_B b + \beta_{VB}(v\cdot b)) before the two-column layout, with a sentence noting it's the same model as @eq-hers-strat under a different coding, and updated the caption to "Regression parameter estimates for [Model @eq-hers-strat-interact]".

Checks: lintr::lint() ✓; spelling::spell_check_package() ✓; isolated HERS render ✓ — @eq-hers-strat-interact resolves, Table 7 now carries the equation-referencing caption, no broken cross-references.

@claude please re-review.


Generated by Claude Code

@claude

claude Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Claude finished @d-morrison's task in 3m 12s —— View job


Code Review — PR #381 (re-review after 380c7b4)

  • Gather context and read changed files
  • Verify round-3 findings are resolved
  • Check for new issues
  • Post findings

Both round-3 findings resolved ✓

# Finding Status
1 Redundant library(plotly) in _sec_hers_data.qmd ✅ Fixed — removed; all plotly calls use plotly:: prefix
2 tbl-hers-lm-statin-interact caption lacked equation cross-reference ✅ Fixed — caption now reads "Regression parameter estimates for [Model @eq-hers-strat-interact]"; @eq-hers-strat-interact is defined just above the two-column table layout

Remaining issues (new)

1. Interaction model 3D figure captions don't reference @eq-hers-interact (_sec_hers_lm_interact.qmd:80–81, 114–115)

The parallel-planes figure captions both reference the model equation:

Fitted regression surface from [Model @eq-hers-parallel] (patients not taking statins)
Fitted regression surface from [Model @eq-hers-parallel] (patients taking statins)

The interaction model figure captions do not:

Interaction model regression surface for `hers` data (patients not taking statins)
Interaction model regression surface for `hers` data (patients taking statins)

Fix: update both interaction figure captions to read "Fitted regression surface from [Model @eq-hers-interact] (patients not taking statins)" and "(patients taking statins)" to match the parallel-planes pattern.

Fix this →


2. Redundant library(ggplot2) in diagnostics subfile (_sec_hers_lm_diagnostics.qmd:14)

library(ggplot2)

ggplot2 is already attached via library(tidyverse) in _sec_linreg_understanding.qmd, which renders before the assess section. All other HERS subfiles either use :: namespacing or have a genuinely necessary library() call (e.g., library(GGally) for unqualified ggpairs()). This is the only HERS subfile with a library() call for something already loaded. Since the diagnostics ggplot calls are all unqualified, the options are:

  • Remove library(ggplot2) (relies on tidyverse being loaded upstream — fragile)
  • Replace with ggplot2::ggplot(...), ggplot2::geom_point(...), etc. (verbose but self-contained)

Neither change is urgent, but the current state is inconsistent with the rest of the HERS subfiles.


Positive notes

  • @eq-hers-strat-interact is correctly defined before the two-column table layout, and the surrounding prose ("Equivalently, we can fit the same model using reference coding…") contextualises the relationship between the two parameterisations.
  • regress3d pinned to commit SHA 158d63c in DESCRIPTION — confirmed stable.
  • WORDLIST is sorted with no duplicates; 9 new HERS terms are all justified.
  • All 7 HERS subfiles are clean of leading section headings.
  • library(plotly) removed from _sec_hers_data.qmd; library(GGally) correctly retained since ggpairs() is called unqualified.
  • AIC/BIC and deviance tables each produce a single output under their div anchors.
  • @tbl-hers-lrt cross-reference in GOF notes correctly replaces a forward "below" reference.

| Branch |

…p redundant library(ggplot2)

- both interaction-model 3D figure captions now read 'Fitted regression
  surface from [Model @eq-hers-interact]', matching the parallel-planes
  figure caption convention
- remove redundant library(ggplot2) from the diagnostics subfile; ggplot2
  is loaded globally via r-config (like every other chapter subfile)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011CqGbHzLqKVGnbEpLEdadG
@claude

claude Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

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


Code Review — PR #381 (re-review after 1118c16)

  • Gather context
  • Verify round-4 findings are resolved (1118c16)
  • Check for new issues
  • Post findings

Both round-4 findings resolved ✓

# Finding Status
1 Interaction model 3D figure captions lacked @eq-hers-interact reference ✅ Fixed — fig-hers-interact-fit-no (line 80) and fig-hers-interact-fit-yes (line 114) now read "Fitted regression surface from [Model @eq-hers-interact] (patients not/taking statins)"
2 Redundant library(ggplot2) in _sec_hers_lm_diagnostics.qmd ✅ Fixed — removed; all ggplot2 calls rely on tidyverse loaded upstream, consistent with _sec_hers_lm_diagnostics_lm2.qmd

No new issues found

All 8 HERS subfiles and the two parent includes are clean:

  • _sec_hers_data.qmdlibrary(GGally) correctly retained (needed for unqualified ggpairs()); library(plotly) correctly removed; all plotly calls are plotly::-qualified.
  • _sec_hers_lm_parallel.qmd — no library() calls; hers_no/hers_yes/hers_plot_data defined once here and reused downstream.
  • _sec_hers_lm_interact.qmd — no redundant object redefinitions; both 3D figure captions reference @eq-hers-interact; tbl-hers-lm2 caption references @eq-hers-interact.
  • _sec_hers_lm_diagnostics.qmd — no library() calls; consistent with lm2 diagnostics file.
  • _sec_hers_lm_diagnostics_lm2.qmd — clean; hers_lm2 in scope from earlier include.
  • _sec_hers_lm_gof.qmd — AIC/BIC and deviance each produce a single knitr::kable() output under their div anchors; @tbl-hers-lrt forward-reference cross-link correct.
  • _sec_hers_lm_model_selection.qmdlmtest::lrtest() qualified; :::{#tbl-hers-lrt} div with caption.
  • _sec_hers_lm_stratified.qmd — both equations displayed (@eq-hers-strat and @eq-hers-strat-interact); both table captions reference their respective equations; V=v notation consistent on both LHS and RHS; age omission explained; #| code-fold: true on all chunks.
  • regress3d in DESCRIPTION — pinned to commit SHA 158d63cee6d823eccb5b0fc45e11f013ea0097d7.
  • WORDLIST — 9 new entries (Statins, bv, drinkany, drk, nw, sciid, smk, tabset), no duplicates.

This PR is ready to merge.


| Branch |

Copy link
Copy Markdown
Owner

Round 4 — addressed both new findings (commit 1118c16):

  1. Address — both interaction-model 3D figure captions now read "Fitted regression surface from [Model @eq-hers-interact]", matching the parallel-planes figure convention.
  2. Address — removed the redundant library(ggplot2) from _sec_hers_lm_diagnostics.qmd; ggplot2 is loaded globally via r-config.qmd (as in every other chapter subfile), so the call was redundant.

Checks: lintr::lint() ✓; spelling::spell_check_package() ✓; isolated HERS render ✓ (no broken cross-references; @eq-hers-interact resolves in the captions).

@claude please re-review.


Generated by Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

👀 Picked up by workflow run #27815436964. R/Quarto/renv setup runs first (~3-5 min); Claude itself responds after that.

@github-actions

Copy link
Copy Markdown
Contributor

I now have everything I need for a thorough review. Here's my assessment:


Round 4 review — two findings remain

1. _sec_hers_lm_stratified.qmdhers_lm_strat missing na.action = na.exclude

Every other hers model in this PR (hers_lm1, hers_lm2, hers_lm_statins_interact) sets na.action = na.exclude. The pipe-based hers_lm_strat (line 37) doesn't:

hers_lm_strat <-
  hers |>
  lm(
    formula = LDL ~ statins + statins:BMI - 1,
    data = _
    # na.action = na.exclude  ← missing
  )

With the default na.omit, fitted values and residuals drop the excluded rows entirely instead of inserting NA placeholders. If anyone runs diagnostics on hers_lm_strat later, the length mismatch will be surprising. One-line fix:

hers_lm_strat <-
  hers |>
  lm(
    formula = LDL ~ statins + statins:BMI - 1,
    data = _,
    na.action = na.exclude
  )

2. _sec_hers_lm_parallel.qmd — model fitting and data prep bundled in the table chunk

The {#tbl-hers-lm1} chunk (lines 18–35) does three things under code-fold: true: fits hers_lm1, defines hers_plot_data, and prints the parameter table. When a student unfolds the table, they see model-fitting and data-filtering code mixed with the table-display code, which is confusing. The downstream 3D figure chunks depend on hers_plot_data being set here.

A cleaner split:

# #| include: false  (hidden setup)
hers_lm1 <- lm(
  formula = LDL ~ age + BMI,
  data = hers,
  na.action = na.exclude
)
hers_plot_data <- hers |>
  dplyr::filter(!is.na(age), !is.na(BMI), !is.na(LDL))

…then the {#tbl-hers-lm1} chunk only prints the table. This is the same #| include: false pattern used in _sec_hers_data.qmd for loading hers.


Everything else looks good

  • \deriv macro is defined at latex-macros/macros.qmd:188 (\frac{\partial}{\partial #1}) — the usage in _sec_hers_lm_interact.qmd is correct
  • All cross-references resolve: @eq-hers-parallel, @eq-hers-interact, @eq-hers-strat, @eq-hers-strat-interact, @tbl-hers-lrt — no broken or undefined labels
  • All table and figure divs use {#tbl-...} / {#fig-...} format (not chunk-option captions)
  • Faceted diagnostic and fit plots all use labeller = label_both
  • GGally::wrap() is namespace-qualified ✓; library(GGally) in the key-vars chunk is genuinely needed (unqualified ggpairs()) ✓
  • No redundant library() calls in parallel or interact files ✓
  • Stratified formula β_{no}(1-v) + β_{no,B} b(1-v) + β_{yes} v + β_{yes,B} bv correctly matches LDL ~ statins + statins:BMI - 1
  • _subfiles/shared/_sec_hers_intro.qmd exists on the branch ✓
  • WORDLIST additions (sciid, bv, drinkany, drk, smk, nw, Statins, tabset) all have clear sources in the new content ✓
  • Heading hierarchy and .smaller usage is internally consistent in both _sec_linreg_understanding.qmd and _sec_linreg_assess.qmd
  • The @tbl-hers-lrt cross-reference in _sec_hers_lm_gof.qmd is the correct Quarto approach (replacing a directional "below") ✓

informational: _sec_hers_data.qmd is 149 lines against the ~100-line guideline. The content is coherent (intro callout, data-loading chunks, panel-tabset with three display modes, data notation), so splitting isn't urgent, but it's worth keeping in mind for future edits.

— posted by @claude post-step from workflow run

Every other hers model (hers_lm1, hers_lm2, hers_lm_statins_interact) sets
na.action = na.exclude; hers_lm_strat used the default na.omit. na.exclude
pads fitted values/residuals with NA placeholders so lengths align with the
data, matching the rest of the chapter.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011CqGbHzLqKVGnbEpLEdadG

Copy link
Copy Markdown
Owner

Two reviews landed on 1118c16: the claude-code-review workflow reported "No new issues found — ready to merge," while the agent post-step raised two more. Dispositions:

  • Address (commit 1ca8d36) — added na.action = na.exclude to hers_lm_strat. The other three models (hers_lm1, hers_lm2, hers_lm_statins_interact) all set it; this one used the default na.omit. na.exclude pads fitted/residuals with NA so lengths align with the data — a real consistency improvement.

  • Rebut — the suggestion to move hers_lm1 fitting + hers_plot_data out of the {#tbl-hers-lm1} chunk into a hidden #| include: false setup chunk. All three HERS model tables (_sec_hers_lm_parallel, _sec_hers_lm_interact, _sec_hers_lm_stratified) fit their model inside the foldable code-fold: true table chunk — that's the consistent pattern across this PR (and the broader chapter). Hiding the lm() call behind include: false would (a) make the parallel subfile inconsistent with its two siblings, and (b) remove pedagogically useful model-fitting code that students can currently reveal by unfolding — which matters in a regression textbook. If a hidden-setup pattern is preferred, it should be applied chapter-wide as a separate refactor rather than to one of three parallel sections here. (The claude-code-review workflow assessed this same commit as ready to merge.)

  • Acknowledge (informational) — _sec_hers_data.qmd at 149 lines vs. the ~100-line soft guideline. Non-urgent; the content (intro callout, data load, three-mode panel-tabset, data notation) is cohesive, so I'm leaving it rather than splitting mid-review.

Checks: lintr ✓, spelling ✓, isolated render ✓.

@claude please re-review.


Generated by Claude Code

@claude

claude Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

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


Code Review — PR #381 (re-review after 1ca8d36)

  • Gather context and read changed files
  • Verify na.action = na.exclude fix in hers_lm_strat
  • Check for any remaining issues
  • Post findings

The one outstanding finding is resolved ✓

# Finding Status
1 hers_lm_strat missing na.action = na.exclude ✅ Fixed — _sec_hers_lm_stratified.qmd:40 now sets na.action = na.exclude, matching every other hers model in this PR

Rebuttal accepted

The suggestion to split model fitting out of the {#tbl-hers-lm1} chunk into a hidden setup chunk is correctly rebutted. All three HERS model table chunks (tbl-hers-lm1, tbl-hers-lm2, both stratified tables) follow the same pattern: model fit + table print inside a single code-fold: true block. The parent _sec_linreg_understanding.qmd does the same for the birthweight models. Changing one of the three would create an asymmetry and would remove pedagogically useful model-fitting code from students' view.


No new issues found

All 8 HERS subfiles are clean:

  • _sec_hers_data.qmdlibrary(GGally) retained (needed for unqualified ggpairs()); no library(plotly); all plotly calls plotly::-qualified.
  • _sec_hers_lm_parallel.qmd — no library() calls; hers_lm1, hers_plot_data, hers_no, hers_yes defined once and reused downstream; na.action = na.exclude set; forward reference to stratified section removed.
  • _sec_hers_lm_interact.qmd — no redundant object redefinitions; both 3D figure captions reference @eq-hers-interact; table caption references @eq-hers-interact; na.action = na.exclude set.
  • _sec_hers_lm_diagnostics.qmd — no library() calls; all figures faceted by statins with label_both; code-fold: true throughout.
  • _sec_hers_lm_diagnostics_lm2.qmd — same structure; hers_lm2 in scope from earlier include.
  • _sec_hers_lm_gof.qmd — AIC/BIC and deviance each produce a single knitr::kable() output under their div anchors; @tbl-hers-lrt cross-reference correctly replaces "below".
  • _sec_hers_lm_model_selection.qmdlmtest::lrtest() namespace-qualified; :::{#tbl-hers-lrt} div with caption.
  • _sec_hers_lm_stratified.qmd — both equations displayed (@eq-hers-strat, @eq-hers-strat-interact); both table captions reference their equations; LHS notation consistent (V=v); age omission explained; na.action = na.exclude on both models; code-fold: true on all chunks.

Parent includes (_sec_linreg_understanding.qmd, _sec_linreg_assess.qmd) correctly place all ### headings at the include sites; no ### headings appear inside any subfile.

This PR is ready to merge.


| Branch

@github-actions

Copy link
Copy Markdown
Contributor

👀 Picked up by workflow run #27842275070. R/Quarto/renv setup runs first (~3-5 min); Claude itself responds after that.

@github-actions

Copy link
Copy Markdown
Contributor

I've now read the full diff. Let me write the review.


Code Review — PR #381 (round 5, commit 1ca8d36)

Overall: The rebuttals and acknowledgments in the previous comment are well-reasoned. The code is in good shape. Only one new issue found.


New finding

hers_scatter_data missing !is.na(statins) filter (_sec_hers_data.qmd, line ~84)

hers_scatter_data <- hers |>
  dplyr::filter(!is.na(age), !is.na(BMI), !is.na(LDL))
#                                            ^ statins not excluded

statins is immediately used as color in plotly::plot_ly() with the discrete color map c("no" = "steelblue", "yes" = "darkorange"). If any row has statins == NA (possible after as_factor() on the Stata-labelled variable), as.character(NA)"NA", which matches neither color key and can produce a plotly warning or an unexpected third color in the legend.

Suggested fix:

hers_scatter_data <- hers |>
  dplyr::filter(!is.na(age), !is.na(BMI), !is.na(LDL), !is.na(statins))

Previously raised items — dispositions confirmed

Item Disposition
na.action = na.exclude on hers_lm_strat ✅ Addressed in 1ca8d36
Move hers_lm1 fit to hidden setup chunk ✅ Rebut is sound — foldable model-fitting code is pedagogically useful and consistent with all three HERS table subfiles
_sec_hers_data.qmd at 149 lines ✅ Acknowledged; content is cohesive
library(GGally) in figure chunk ✅ Justified — ggpairs() is called unqualified
@tbl-hers-lrt cross-reference from GOF section ✅ Quarto resolves cross-references across the full document; not a forward-reference problem
Cross-subfile dependencies (hers_no, hers_yes, hers_plot_data) ✅ Intentional and consistent with the sibling-subfile pattern

The statins filter is a minor correctness issue but worth fixing before merge, since it affects a figure in the data introduction section. Everything else looks ready.

— posted by @claude post-step from workflow run

…esthetic

statins is used as the discrete color in the 3D scatter (color map
c(no=steelblue, yes=darkorange)); an NA would render as an unmapped 'NA'
color. Add !is.na(statins) to the filter to guard against that. The current
data has no NA statins, so the rendered figure is unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011CqGbHzLqKVGnbEpLEdadG

Copy link
Copy Markdown
Owner

Addressed the agent post-step's new finding (commit 20afea0):

  • Addresshers_scatter_data (_sec_hers_data.qmd) now also filters !is.na(statins) before statins is used as the discrete color aesthetic, guarding against an unmapped "NA" color in the 3D scatter. Verified against the data: there are currently 0 NA statins values, so the rendered figure is unchanged (2747 rows, levels no/yes only) — this is a defensive guard, not a behavioral change.

Checks: lintr ✓; filter logic sanity-checked against inst/extdata/hersdata.dta.

(The claude-code-review workflow reported this same commit's predecessor as "ready to merge"; this resolves the one item the agent post-step raised.)

@claude please re-review.


Generated by Claude Code

@claude

claude Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Claude finished @d-morrison's task in 2m 50s —— View job


Code Review — PR #381 (re-review after 20afea0)

  • Gather context
  • Read changed files
  • Verify !is.na(statins) fix in hers_scatter_data
  • Check for new issues
  • Post findings

Round-5 finding resolved ✓

# Finding Status
1 hers_scatter_data missing !is.na(statins) guard before statins-as-color usage ✅ Fixed — _sec_hers_data.qmd:59 now filters !is.na(statins) alongside age/BMI/LDL

One minor observation (not blocking)

hers_plot_data in _sec_hers_lm_parallel.qmd:28–29 still doesn't include !is.na(statins):

hers_plot_data <- hers |>
  dplyr::filter(!is.na(age), !is.na(BMI), !is.na(LDL))

hers_plot_data is then used to derive hers_no / hers_yes (via statins == "no" / "yes" filters, which silently drop any NA-statins rows) and to compute the 3D regression surface extent. Since the current data has 0 NA statins, this is not a practical problem, and the behavior is defensibly different from hers_scatter_data (which explicitly needs the guard because statins drives the color map). Mentioning for completeness; not a blocker.


No other issues found

All 8 HERS subfiles are clean:

  • _sec_hers_data.qmd!is.na(statins) now guarded in hers_scatter_data; library(GGally) retained; library(plotly) removed; all plotly calls plotly::-qualified.
  • _sec_hers_lm_parallel.qmd — no library() calls; hers_lm1, hers_plot_data, hers_no, hers_yes defined once; na.action = na.exclude set; forward reference to stratified section removed.
  • _sec_hers_lm_interact.qmd — no redundant object redefinitions; both 3D captions reference @eq-hers-interact; table caption references @eq-hers-interact; na.action = na.exclude set.
  • _sec_hers_lm_diagnostics.qmd — no library() calls; all figures faceted with label_both; code-fold: true throughout.
  • _sec_hers_lm_diagnostics_lm2.qmd — same structure; hers_lm2 in scope from earlier include.
  • _sec_hers_lm_gof.qmd — AIC/BIC and deviance each produce a single knitr::kable() output under their div anchors; @tbl-hers-lrt cross-reference correctly replaces a directional "below".
  • _sec_hers_lm_model_selection.qmdlmtest::lrtest() qualified; :::{#tbl-hers-lrt} div with caption; code-fold: true.
  • _sec_hers_lm_stratified.qmd — both equations displayed; both table captions reference their equations; V=v notation consistent on both LHS and RHS; age omission explained; na.action = na.exclude on both models; code-fold: true on all chunks.

Parent includes (_sec_linreg_understanding.qmd, _sec_linreg_assess.qmd) correctly place all ### headings at the include sites; no subfile begins with a heading. regress3d is pinned to commit SHA 158d63c in DESCRIPTION. WORDLIST is sorted with no duplicates.

This PR is ready to merge.


| Branch

@github-actions

Copy link
Copy Markdown
Contributor

👀 Picked up by workflow run #27842747585. R/Quarto/renv setup runs first (~3-5 min); Claude itself responds after that.

Copy link
Copy Markdown
Owner

Thanks — noting the round-7 verdict is "ready to merge."

On the one remaining non-blocking observation (hers_plot_data not filtering !is.na(statins)): I'm acknowledging without changing it, for the reasons the review itself gives — the behavior is "defensibly different" from hers_scatter_data (there, statins drives the discrete color map and an unmapped NA would show; here hers_plot_data only feeds the surface extent and is split into hers_no/hers_yes via statins == "no"/"yes" filters, which already exclude any NA rows). The current data also has 0 NA statins. So there's no correctness gap to close, and I'd rather not churn the PR further. Happy to add the guard for symmetry if you'd prefer it.

This PR is at a clean, ready-to-merge verdict from both review workflows.


Generated by Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

I've reviewed the full diff across all 11 changed files. Here is my assessment:


Round 5 review — no new issues

The code on 20afea0 is clean. Here is what I checked:

Cross-file R environment dependencieshers_lm1 / hers_plot_data / hers_no / hers_yes are defined in _sec_hers_lm_parallel.qmd (under # Understanding…), and hers_lm2 in _sec_hers_lm_interact.qmd. Both are consumed later in # Assessing model fit (_sec_hers_lm_diagnostics.qmd, _sec_hers_lm_diagnostics_lm2.qmd, _sec_hers_lm_gof.qmd). Render order in chapters/Linear-models-overview.qmd is: Understanding → Fitting → Assessing → Inference, so the dependencies resolve correctly. ✓

na.action = na.exclude consistency — all four HERS models (hers_lm1, hers_lm2, hers_lm_strat, hers_lm_statins_interact) now carry it. ✓

Diagnostic mutate(.fitted, .resid) alignmenthers_lm1/hers_lm2 are fitted on data = hers (full data frame), so fitted() / residuals() with na.exclude return padded vectors of nrow(hers) and the mutate aligns correctly. ✓

Latest hers_scatter_data fix!is.na(statins) guard added; currently 0 NA statins rows, so no behavioral change, but consistent with the defensive filter applied for the discrete color aesthetic. ✓

@tbl-hers-lrt forward cross-reference — used in _sec_hers_lm_gof.qmd, defined later in _sec_hers_lm_model_selection.qmd. Quarto resolves cross-references in a second pass, so forward refs are fine. ✓

GOF tables — each {#tbl-…} div produces a single knitr::kable() output. ✓

Figure captions — both interaction-model 3D figures read "Fitted regression surface from [Model @eq-hers-interact]", matching the parallel-planes convention. ✓

WORDLISTSubmodule correctly moved from the lowercase section to the uppercase section; all eight new entries (Statins, bv, drinkany, drk, nw, sciid, smk, tabset) are in correct ASCII-sort positions. ✓

DESCRIPTIONregress3d Remotes entry is pinned to commit SHA 158d63ce… (not a mutable branch ref). ✓

No new issues found. The claude-code-review workflow's "ready to merge" verdict on the predecessor commit stands. This is good to go pending CI.

— posted by @claude post-step from workflow run

@github-actions

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
🚀 Preview available at: https://d-morrison.github.io/rme/pr-preview/pr-381/

Built to branch gh-pages at 2026-06-19 18:58 UTC.

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.

add an example model with the HERS dataset for linear regression

4 participants