GAM chapter (closes #775)#779
Conversation
|
@claude review |
|
👀 Picked up by workflow run #26159255775. R/Quarto/renv setup runs first (~3-5 min); Claude itself responds after that. |
|
@claude review |
|
👀 Picked up by workflow run #26177399193. R/Quarto/renv setup runs first (~3-5 min); Claude itself responds after that. |
|
Round-3 review came back with 12 items across 3 priority tiers, but on verification most don't match the current source. Per-item audit: Hallucinated (not present in
Actionable (fixed in
Deferred (acknowledged in PR description):
Skipped per CLAUDE.md:
Triggering another review for confirmation. |
|
@claude review |
|
👀 Picked up by workflow run #26186262395. R/Quarto/renv setup runs first (~3-5 min); Claude itself responds after that. |
|
@claude review |
|
👀 Picked up by workflow run #26196646462. R/Quarto/renv setup runs first (~3-5 min); Claude itself responds after that. |
|
@claude review |
|
👀 Picked up by workflow run #26200740705. R/Quarto/renv setup runs first (~3-5 min); Claude itself responds after that. |
|
@claude review |
|
@claude review |
|
👀 Picked up by workflow run #26203430876. R/Quarto/renv setup runs first (~3-5 min); Claude itself responds after that. |
|
I'll analyze this and get back to you. |
|
I'll analyze this and get back to you. |
|
todo: change "Show R code" to "Show code" |
|
I'll analyze this and get back to you. |
|
I'll analyze this and get back to you. |
|
@claude review |
|
👀 Picked up by workflow run #26915839558. R/Quarto/renv setup runs first (~3-5 min); Claude itself responds after that. |
|
Claude Code CLI (local session) is working on this — paws off until I'm done. |
|
The latest I re-audited every finding from the last completed review and the inline comments; all are already resolved in the current branch:
Fresh review now running on the merge commit. |
|
👀 Picked up by workflow run #26917221781. R/Quarto/renv setup runs first (~3-5 min); Claude itself responds after that. |
|
I'll analyze this and get back to you. |
|
Now I have enough to write a thorough review. Code Review — PR #779: GAM chapter (closes #775)OverviewThis PR adds a substantial new chapter on Generalized Additive Models (11 sections, 2,356 lines, 16 new files). The structure follows ISL Ch. 7, the content is mathematically sound, and the pedagogical flow is well-designed. The PR description is transparent about the known gaps (no course-specific dataset, incomplete exercises). Below are the findings. Must-fix before merge1. Nine subfiles are missing trailing newlines Every new
POSIX text files must end in a newline. Add one to each. 2.
Should-fix (convention violations)3. Inconsistent code block syntax in The worked example section uses plain backtick- 4. In :::: {.content-hidden}
::: callout-warning
This section is illustrative only …
:::
::::
Nice-to-have / minor5.
6.
Content quality (no action needed, FYI)The mathematics is correct throughout:
The section on tensor-product smooths ( Citation check: Both — posted by @claude post-step from workflow run |
|
I'll analyze this and get back to you. |
|
I'll analyze this and get back to you. |
Initial draft of a chapter on generalized additive models. Squashed from the original 69-commit history because the action's recursive submodule fetch was failing on commits that referenced a now-deleted latex-macros SHA. See the PR description for the chapter outline, new bib entries, and caveats.
79a7d81 to
ef26f15
Compare
|
Claude finished @d-morrison's task in 5m 56s —— View job Code Review — GAM Chapter (PR #779)
Resolved since 2026-06-03 reviewThe two code-correctness bugs flagged in prior rounds are confirmed fixed:
Still open: must-fix1. Nine subfiles missing trailing newlinesPOSIX text files must end with
Fix: add a single 2.
|
| # | Severity | File | Line | Issue |
|---|---|---|---|---|
| 1 | Must-fix | 9 subfiles | (end-of-file) | Missing trailing newlines — carry-over |
| 2 | Must-fix | _sec_gam_example.qmd, _sec_gam_inference.qmd |
20, 116, 77, 97 | gratia used but not in DESCRIPTION — carry-over |
| 3 | Should-fix | _sec_gam_example.qmd |
1–7 | .content-hidden hides placeholder warning from everyone — carry-over |
| 4 | Should-fix | 7 subfiles | throughout | Display R blocks use markdown ```r not Quarto {r} — carry-over |
| 5 | Low | _sec_lab.qmd |
36 | Orphaned # noqa: E402 on commented-out import — new |
| 6 | Low | _sec_gam_fitting.qmd |
43–50 | s() table missing id argument — carry-over |
Items 1 and 2 are blockers for a clean build on a fresh environment. Items 3–6 are convention/clarity issues. The mathematical content and code correctness of the chapter remain sound.
…deps
- Add trailing newlines to 9 _subfiles/generalized-additive-models/
files. POSIX requires text files to end with a newline; many tools
(cat, sed, diff, git's last-line tracking) misbehave without one.
- Convert 7 subfiles' display-only ```r blocks to ```{r}\n#| eval: false
so they render as code fences without trying to evaluate. Keeps
the prose-illustration semantics but pulls the snippets into
Quarto's code-display path (gets syntax highlighting + copy
button + per-language config), instead of the plain Pandoc one.
- _sec_gam_example.qmd: remove the outer .content-hidden div around
the callout-warning. The whole point of the warning ("dataset is
illustrative; swap for an Epi 204 case study") is to be visible
to readers; hiding it defeats it. The unwrapped callout shows in
HTML, slides, and PDF, which is the intended audience.
- DESCRIPTION: declare gratia (>=0.9) and mgcv in Suggests. Both
are loaded in the chapter (eval:false), and listing them lets
renv resolve consistent versions when the chapter ever gets
flipped to eval:true.
|
Claude finished @d-morrison's task in 4m 49s —— View job Code Review — GAM Chapter (PR #779)
Resolved since 2026-06-03 review (4 must-fix / should-fix items)Commit
Still open — three low-priority items1.
|
Replace the built-in MASS::Pima.tr placeholder with the course's WCGS coronary-heart-disease cohort, addressing the review concern that the worked example was not an Epi 204 case study (and the student-visible 'illustrative only / replace dataset' callout-warning). - Outcome: incident CHD (chd69); smoothed continuous predictors: age, bmi, chol, sbp. - Load WCGS from the package's bundled inst/extdata/wcgs.rds. - Drop the placeholder callout-warning. - Adjust the narrative to WCGS reality (most smooths near EDF 1, cholesterol the predictor most likely to show curvature) without asserting specific run-dependent numbers. Chunks remain eval:false, consistent with the section's existing style and the book's other WCGS code listings. https://claude.ai/code/session_01NQSLJmjMN22RpwpgJ6Lw6u
|
Claude finished @d-morrison's task in 5m 3s —— View job Code Review — GAM Chapter (PR #779)
Resolved since 2026-06-09 reviewThe WCGS dataset commit (
Previous blockers confirmed still resolved from
Still open — three low-priority items (all carry-overs)1.
|
Built to branch |
…chapter Co-authored-by: d-morrison <2474437+d-morrison@users.noreply.github.com>
1. _sec_lab.qmd:36 — remove the # noqa: E402 trailing the commented-
out pygam import. The import is already commented out, so the
linter suppression is meaningless and just noise.
2. _sec_gams.qmd:95 — the inline `s(age, by = sex)` example was the
one ```r block missed by the prior cleanup commit; convert to
```{r} #| eval: false to match the rest of the chapter.
3. _sec_gam_fitting.qmd: add the `id` argument to the s() argument
table — used to share a single smoothness parameter across two
or more smooths (e.g. s(x1, id = 1) + s(x2, id = 1)). Commonly
needed in multi-predictor epi models.
Also merged origin/main (the branch was 125 commits behind).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: d-morrison <2474437+d-morrison@users.noreply.github.com>
|
Claude finished @d-morrison's task in 4m 50s —— View job Code Review — GAM Chapter (PR #779)
Resolved since 2026-06-09 review (3/3)All three low-priority items from the previous round are confirmed fixed:
All must-fix blockers from prior rounds remain resolved:
New findings1. (Very low) Commented-out import in setup block is now just dead code
# from pygam import LinearGAM, LogisticGAM, s as gam_s, f as gam_fThe 2. (Very low) Two remaining references to Pima data in illustration examples
These are not the main worked example (which was correctly updated to WCGS), and both use Summary
The chapter is otherwise clean. All prior must-fix and should-fix items are confirmed resolved. The mathematical content, code correctness, and pedagogical structure remain sound. The two remaining notes are housekeeping items that do not block merge. |
1. _sec_lab.qmd:36 — drop the commented-out pygam import entirely. The active imports of LogisticGAM/gam_f live in the chunks that use them (lines 293, 350), so the setup-block comment was just dead code that puzzled students. 2. _sec_gams.qmd:38 — switch #exm-gam conceptual example from "Pima diabetes data with bmi/glu/age → diabetes" to "WCGS data with age/sbp/chol → chd69" so it matches the worked example's WCGS framing. 3. _sec_local_regression.qmd:34 — switch #exm-local-regression from loess(glu ~ age, MASS::Pima.tr) to loess(chol ~ age, wcgs) for the same reason; pivot the surrounding prose from age 40/glucose to age 50/cholesterol to match. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: d-morrison <2474437+d-morrison@users.noreply.github.com>
|
Claude finished @d-morrison's task in 4m 17s —— View job Code Review — GAM Chapter (PR #779)
Resolved since last review (2/2)Both "very low" items from the 2026-06-18 review are confirmed fixed:
All must-fix blockers from prior rounds remain resolved:
New finding (low priority)
|
DESCRIPTION:49 declares gratia (>= 0.9) in Suggests, but
renv::snapshot() doesn't auto-detect it because all gratia code
lives in eval:false chunks. Force-record via renv::record('gratia')
so renv::restore() on a fresh clone installs it.
Other Suggests dependencies are already present in renv.lock (mgcv,
ISLR2, boot, etc.), so this brings gratia into line with that
pattern.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: d-morrison <2474437+d-morrison@users.noreply.github.com>
|
Claude finished @d-morrison's task in 4m 31s —— View job Code Review — GAM Chapter (PR #779)
Resolved since last review (all 3 carry-overs)Commits
All must-fix and should-fix items from prior rounds confirmed still resolved:
Citation auditAll six BibTeX keys introduced by this PR resolve in Fresh review: no new issues foundThe chapter is clean. All must-fix blockers and should-fix items from all prior rounds are confirmed resolved. The mathematical content, code correctness, and pedagogical structure of the 11-section chapter are sound. |

Summary
Initial draft of a new chapter on generalized additive models, addressing d-morrison/rme#775. Placed at the end of the GLMs part of the book; structure follows ISL Ch 7 progression with extensions from Wood 2017 for the penalized-likelihood / REML / inference content.
Chapter outline (11 sections)
splines::ns()/splines::bs().by =,te()).mgcv::gam(),s()arguments, REML vs GCV,gam.check(),plot.gam().summary(), EDFs, smooth-term p-value caveats per @wood2013pvalues, AIC, when to prefer a GLM.MASS::Pima.tr, from glm baseline → gam → partial-effect plots → response-scale plot.New bib entries
@aune2016bmi— Aune et al. 2016 BMJ BMI/mortality meta-analysis@gasparrini2015mortality— Gasparrini et al. 2015 Lancet temperature/mortality multi-country study@wood2013pvalues— Wood 2013 Biometrika on smooth-term p-valuesCaveats / known follow-ups
MASS::Pima.tr(built-in, no extra data dependency). The intro note flags that this should be swapped for a course-appropriate Epi 204 case study before the chapter is finalized._exr-*.qmdexercises subfile; one should be added once the chapter content stabilizes.Pre-commit checks
quarto render chapters/generalized-additive-models.qmd --to htmllintr::lint()on all 11 new subfiles (no lints)spelling::spell_check_package()(no errors)Test plan
@def-*,@exm-*,@thm-*label resolves.MASS::Pima.trfor a course dataset in this PR or a follow-up.🤖 Generated with Claude Code