Skip to content

Solve parts (b) and (c) of the Wage polynomial exercise; scale age in the Python CV - #1399

Open
d-morrison wants to merge 3 commits into
mainfrom
claude/gam-applied-solutions
Open

d-morrison wants to merge 3 commits into
mainfrom
claude/gam-applied-solutions

Conversation

@d-morrison

Copy link
Copy Markdown
Collaborator

Before: exr-wage-polynomial-cv asks for a confidence band (b) and a cross-validated step function (c), but its solution only did (a). The Python solution passed raw age**10 to least squares, which is ill-conditioned, and its folds were not shuffled.

After: the R and Python solutions both do (a), (b) and (c). Python standardizes age before taking powers, as the R solution's poly() does, and uses shuffled folds. R and Python pick the same answers: degree 9 and 7 cut-points.

How: only _subfiles/generalized-additive-models/_exr-gams-applied.qmd changes. I rendered chapters/generalized-additive-models.qmd to HTML with no warnings; the R output reads "CV-selected degree: 9" and "CV-selected number of cut-points: 7". The Python chunk is not run in the rme render (eval: false), so I ran it locally against the same Wage data and got the same two values. typos is clean on the file. Part of the drift audit in Morrison-Lab/lds#1214, which listed these as follow-ups to rme#1393.

Differences from the source (ISLRv2 Applied 6):

Item Kind Decision
Solution omitted (b) and (c) accidental added
Python PolynomialFeatures on raw age accidental standardize first
Step-function cut-points counted as intervals clarified K cut-points make K + 1 intervals, stated in the text

Generated by Claude Code

… in the Python CV

The exercise asks for a confidence band (b) and a cross-validated step function (c),
but the solution only did (a). The Python solution also fed raw age**10 to least squares,
which is ill-conditioned; it now standardizes age first, as the R solution's poly() does.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nh61VJUp2V6tGvtkZ3BZKF

Copy link
Copy Markdown
Collaborator Author

link-checker / link-checker failed on its only error: [503] https://oeis.org/wiki/List_of_LaTeX_mathematical_symbols (at 197:34) in chapters/intro-to-R.qmd. This PR does not touch that file, so the failure is an external outage of oeis.org, not this change. I am using the one allowed re-run of the failed job now.


Generated by Claude Code

Copy link
Copy Markdown
Collaborator Author

The re-run of link-checker / link-checker failed again on the same oeis.org 503. A fix already exists in rme#1398, which excludes that one URL in lychee.toml. I am not copying it into this PR, because rme keeps .github/CI/config changes out of content PRs. Once rme#1398 merges I will merge main into this branch so the check runs against it.


Generated by Claude Code

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nh61VJUp2V6tGvtkZ3BZKF
@github-actions

Copy link
Copy Markdown
Contributor

Claude finished review — View run


This confirms the only change since the last review round is exactly the fix the author claimed: Wage_cut → wage_cut (both occurrences, lines 75–76 in the current file), resolving the single blocking finding from the prior round.

I re-checked this fix against the rule it was flagged under: .lintr.R's object_name_linter (snake_case_ACROs1) requires each underscore-delimited segment to be fully lowercase or fully uppercase. wage_cut satisfies this (both segments lowercase), so the lint violation is resolved.

I also double-checked that no other part of the diff changed in this round — the full PR diff (_subfiles/generalized-additive-models/_exr-gams-applied.qmd) is otherwise identical to what was verified in the prior round (numerics, links, Python/R logic), and nothing new was introduced that needs re-verification.

Verdict

Ready for merge — the prior round's sole blocking finding (Wage_cut violating the lintr snake_case rule) has been fixed exactly as described, confirmed by diffing 7ab62686..73795ae6; see the prior review at #1399 (comment) (posted 2026-10-21T19:27:02Z, run https://github.com/Morrison-Lab/rme/actions/runs/39660426957) for the full original verification (numerics reproduced against real Wage data, link resolution, no hallucinations). No new issues found in this round.

Structured Review Data (JSON)
{
  "schema_version": "1.1",
  "reviewer": "claude",
  "commit_sha": "73795ae619e2be5dd558c85f8479c12d63f54b94",
  "verdict": "CLEAN",
  "findings": [],
  "detailed_assessment": "The only change since the last round, in _subfiles/generalized-additive-models/_exr-gams-applied.qmd, renames Wage_cut to wage_cut, resolving the previously confirmed lintr object-name violation with no new defect introduced.",
  "holistic_assessment": "The fix satisfies the prior blocking requirement exactly as scoped, integrates cleanly with the already-verified CV and prediction logic, and carries no regression risk since no other lines changed in this round."
}

Reviewed commit: 10adecd

@github-actions

Copy link
Copy Markdown
Contributor

💰 Cost: $0.1995 (review) — run

@github-actions

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
🚀 Preview available at: https://Morrison-Lab.github.io/rme/pr-preview/pr-1399/

Built to branch gh-pages at 2026-10-21 19:45 UTC.

This branch has not been deployed

No deployments
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