Skip to content

Concept map: dependency diagram + descendants table for all definitions and results (#872)#873

Open
github-actions[bot] wants to merge 24 commits into
mainfrom
claude/issue-872-20260603-013639
Open

Concept map: dependency diagram + descendants table for all definitions and results (#872)#873
github-actions[bot] wants to merge 24 commits into
mainfrom
claude/issue-872-20260603-013639

Conversation

@github-actions

@github-actions github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Stacked PR for #872 (base: claude/issue-872-divs → PR #875).

What this adds

A new appendix, chapters/concept-map.qmd, that visualizes how every definition and result in the notes depends on the others:

  • Dependency diagram (@fig-concept-map): a color-by-type graph of the results that participate in ≥1 dependency link, with node size proportional to the number of descendants so the most foundational results stand out (the largest hubs are labeled).
  • Descendants table (@tbl-descendants): for every result with ≥1 descendant, its direct and indirect descendants as live @ref cross-reference links, sorted by number of direct descendants.

How it works (saved artifact, not re-scanned each render)

  • data-raw/callout-graph.R scans the .qmd sources for def/thm/lem/cor/prp callouts and the @type-id references inside them, computes the dependency graph + descendant lists, and saves it to inst/extdata/callout-graph.rds.
  • The appendix reads that saved .rds (via here::here(...)) rather than re-scanning on every render. Re-run Rscript data-raw/callout-graph.R whenever divs are added, removed, or re-titled.

Current snapshot: 433 results, 88 dependency links, 65 results with ≥1 descendant; top hub thm-prob-to-odds (6 direct / 20 total descendants).

Edge attribution

A reference becomes an edge if it sits inside a callout's statement, or inside a proof/solution div that immediately follows a callout (only blank lines / --- / slidebreaks between). References in plain prose don't create edges. This "immediately follows" rule was added after testing the approach in the qwt template repo, where a proof in a separate section was otherwise mis-attributed to the wrong theorem.

Checks

  • quarto render chapters/concept-map.qmd --to html ✅ — figure + 66-row table build
  • lintr ✅ — 0 findings on data-raw/callout-graph.R and chapters/concept-map.qmd
  • spelling::spell_check_package() ✅ — clean
  • Note: the @ref links in the table point to results in other chapters, so they show as unresolved when rendering this chapter in isolation (the single-chapter render CLAUDE.md prescribes). They resolve in the full book/website build, which shares one cross-reference namespace — verified that rme's native theorem crossrefs resolve (e.g. probability.html has zero unresolved refs).

🤖 Generated with Claude Code

@claude

claude Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

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


Code Review: PR #873 — Relationship Diagram for Definitions & Results

Status: Review complete


Summary

This PR is a draft stub with no implementation — the branch has 0 changed files relative to origin/main. There is nothing to review in terms of code changes. The review below instead evaluates the scope of the requested feature, the existing infrastructure it would need to build on, and what a correct implementation would require.


Issue #872 Requirements

The feature asks for:

  1. A diagram (color-coded by type: def, thm, lem, cor, prp) showing the dependency relationships between all labeled callouts, with node size/brightness proportional to the number of descendant results
  2. A table listing all direct and indirect descendants of each item, with hyperlinks, sorted by descendant count
  3. Ensuring all definitions and results have concise titles in their divs

Scale of the Problem

After scanning the repository:

  • 441 labeled callout divs across chapters/ and _subfiles/ (excluding exm, exr, fig, tbl — only def, thm, lem, cor, prp)
  • 152 cross-references (@def-*, @thm-*, @lem-*, @cor-*, @prp-*) in prose/math

Most-referenced items:

  • @cor-fubini-joint (11 refs)
  • @thm-prob-to-odds (8 refs)
  • @thm-fubini (7 refs)

Existing Conventions to Follow

The repo already has small-scale relationship diagrams (e.g., _subfiles/shared/_surv_diagram.qmd) using LaTeX arrow notation (\xleftarrow, \xrightarrow). That approach doesn't scale to 441 nodes.

For a global diagram, the appropriate tools would be:

  • Mermaid (natively supported in Quarto via fenced mermaid blocks) — zero dependencies, but layout control is limited at 441 nodes
  • R + DiagrammeR / visNetwork / igraph in a code chunk — more control, requires adding a package dependency
  • Graphviz (dot format) via the Quarto graphviz engine — renders well at scale

The existing _subfiles/logistic-regression/_fig-expit-logit-diagram-content.qmd and similar files show diagrams are placed in _subfiles/ as includes.


What a Correct Implementation Needs

Step 1 — Extraction script (R/build_callout_graph.R or similar):

  • Walk all .qmd files
  • Extract :::{#TYPE-ID} patterns + their titles (the text after #type-id} on the opening line, or the Quarto title attribute)
  • Extract @TYPE-ID cross-references and which div they appear inside (to record the dependency edge)

Step 2 — Diagram generation (new .qmd file, e.g., chapters/concept-map.qmd):

  • Build adjacency data from the extraction script
  • Render a graph (Mermaid, DiagrammeR, or visNetwork)
  • Color-code nodes by type (def = one color, thm = another, etc.)
  • Size nodes by descendant count

Step 3 — Table:

  • For each node, compute direct and indirect descendants (transitive closure)
  • Render as a sortable Quarto table with hyperlinks to the callout anchors

Step 4 — Title audit:

  • Many divs currently open as :::{#def-foo} with no "Title" attribute — they render with auto-generated headings
  • A complete title attribute (e.g., :::{#def-foo}) is required for the table to be useful

Issues With Current State

  1. Branch is completely empty — no implementation, no extraction script, no chapter file. The PR cannot be merged or reviewed for code quality in its current form.

  2. No title attribute audit — Per requirement 3 of the issue, all callout divs need concise titles. Spot check shows many divs open without a title="" attribute (e.g., :::{#def-mean} in chapters/exploratory-descriptive.qmd).

  3. No dependency on renv.lock — If an R-based graph package (visNetwork, DiagrammeR) is added, it must be added via renv::install() and committed to renv.lock.

  4. Missing chapter in _quarto.yml — Any new concept-map.qmd or similar chapter must be registered in _quarto.yml to be included in the book.

  5. Subfile heading rule — Per CLAUDE.md, subfiles in _subfiles/ must NOT begin with a section heading; headings belong in the parent .qmd. An extraction script would need to be aware of this when attributing cross-references to their parent context.


Recommendation

This is a complex, multi-step feature that requires:

  • A non-trivial extraction script
  • A new chapter file
  • Package dependency management
  • A title audit of 441+ callout divs

The draft PR in its current empty state should remain a draft until at minimum the extraction script and a proof-of-concept diagram with a small subset of nodes (e.g., just the probability chapter) are committed. That would allow meaningful review of the approach before investing in the full 441-node graph.

@github-actions github-actions Bot requested a review from d-morrison June 3, 2026 07:45
@d-morrison

Copy link
Copy Markdown
Owner

Claude Code CLI (local session) is working on this — paws off until I'm done.

New appendix `chapters/concept-map.qmd` visualizes how every definition
and result in the notes depends on the others:

- a color-by-type dependency diagram, with node size proportional to the
  number of descendants so the most foundational results stand out, and
- a table listing each result's direct and indirect descendants (live
  `@ref` cross-reference links), sorted by number of direct descendants.

The graph is built by `data-raw/callout-graph.R`, which scans the `.qmd`
sources and saves the result to `inst/extdata/callout-graph.rds`. The
appendix reads that saved artifact, so the scan does not re-run on every
render — re-run the script when divs are added, removed, or re-titled.

References are attributed to the enclosing callout, or to the callout an
immediately-following proof/solution div belongs to; references that
aren't inside a result (plain prose) don't create edges.

Stacked on the title-audit PR (#875), which gives every node a concise
label. Registered as the final appendix in the book and website configs.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Co-authored-by: d-morrison <2474437+d-morrison@users.noreply.github.com>
@d-morrison d-morrison force-pushed the claude/issue-872-20260603-013639 branch from 61b44fc to 758807a Compare June 3, 2026 09:47
@d-morrison d-morrison changed the base branch from main to claude/issue-872-divs June 3, 2026 09:47
@github-actions github-actions Bot removed the request for review from d-morrison June 3, 2026 09:47
@claude

claude Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

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


Code Review: PR #873 — Concept Map Appendix

  • Gather context
  • Read changed files
  • Review data-raw/callout-graph.R
  • Review chapters/concept-map.qmd
  • Review _quarto-book.yml / _quarto-website.yml
  • Post findings

Summary

Solid implementation. The stack-based .qmd parser is well-structured, the edge direction semantics are consistent throughout, and the pre-built RDS pattern correctly avoids re-scanning on every render. One usability bug (diagram labels), two fragility nits in the extraction script, and a few observations.


Issues

1. Diagram labels show raw IDs, not titles — concept-map.qmd:90-91

aes(label = ifelse(n_desc >= 3, name, "")),

name here is the igraph vertex name, which is the callout ID (e.g. def-random-variable, thm-prob-to-odds). The extraction script already pulls the human-readable heading title into V(core)$title. Showing the ID makes the diagram nearly unreadable without a lookup table.

Fix: Use title instead of name in the label aesthetic:

aes(label = ifelse(n_desc >= 3, title, "")),

Fix this →

2. do.call(rbind, nodes) crashes if nodes is empty — callout-graph.R:125

nodes <- do.call(rbind, nodes)

If no .qmd files are found, or if they contain no matching divs, nodes is list(). In R, do.call(rbind, list()) returns NULL, not a data frame, and every downstream operation (cg$nodes$id, graph_from_data_frame(..., vertices = cg$nodes, ...)) would then fail with an opaque error. This will not occur in the current repo, but it makes the function fragile.

Fix: Add a guard before returning:

nodes <- if (length(nodes)) do.call(rbind, nodes) else
  data.frame(id = character(), type = character(),
             file = character(), stringsAsFactors = FALSE)

Fix this →

3. Proof-detection regex false-positive risk — callout-graph.R:79

is_proof <- grepl("proof|solution", content, ignore.case = TRUE)

content here is the full attribute string after stripping the leading :::+, so a div like :::{#disproof-main-result} would match and incorrectly inherit the preceding callout as its owner. Low probability in this repo, but the pattern should anchor to the class name:

is_proof <- grepl("\\b(proof|solution)\\b", content, ignore.case = TRUE)

The word boundary \\b prevents disproof from matching. {.proof} and {.solution} would still match. Fix this →


Observations (no action required)

Committed binary artifact (inst/extdata/callout-graph.rds): The pre-built RDS approach is the right call — it keeps render times predictable. The script header and the chapter intro both spell out the regeneration requirement. The one maintenance risk is that regenerating the RDS produces a new binary diff on every run (R's serialization includes version info), so the history will accumulate binary blobs. Low concern for a textbook repo, but worth knowing.

Long indirect-descendants cells: For highly foundational items (e.g. cor-fubini-joint with 11+ refs), the "Indirect descendants" column could be very long. In HTML this is fine; in the PDF build (downloads: [pdf] in _quarto-book.yml), an extremely wide pipe-table cell may cause a layout overflow or be silently truncated. Worth checking the PDF output when the table is populated.

distances() called once per node in cg$descendants: For 441 nodes, this is O(441) single-source BFS calls. With a sparse graph this is fast, but ego(ig, order = 1, mode = "out") would give all direct successors in a single pass. Not a problem in practice since this runs offline, but a minor efficiency note.


What Looks Good

  • All three packages (igraph, ggraph, ggrepel) and here are present in renv.lock — no missing dependencies.
  • The stack-based parser correctly handles :::: (4-colon) divs alongside ::: (checked in calc-derivatives.qmd), nested non-callout divs inside callouts, and the blank-line/separator adjacency rule for attached proofs.
  • Edge direction is consistent: from = referenced, to = referencing → outgoing edges from A reach A's descendants. ✓
  • set.seed(204) ensures reproducible stress layout (nice course-number Easter egg).
  • shared-config.qmd relative include path matches the pattern used by all other chapters.
  • Chunk options (code-fold: true, div-format figure and table) follow CLAUDE.md conventions.
  • Chapter placed in appendices: in _quarto-book.yml — correct.

@d-morrison d-morrison changed the title add a diagram showing the relationships between all the definitions and results in the lecture notes Concept map: dependency diagram + descendants table for all definitions and results (#872) Jun 3, 2026
@d-morrison d-morrison self-requested a review June 3, 2026 09:48
Base automatically changed from claude/issue-872-divs to main June 3, 2026 18:19
@github-actions github-actions Bot removed the request for review from d-morrison June 4, 2026 00:10
@claude

claude Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

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


Code Review: PR #873 — Concept Map Appendix

  • Gather context and read PR description
  • Read data-raw/callout-graph.R
  • Read chapters/concept-map.qmd
  • Check _quarto-book.yml / _quarto-website.yml changes
  • Verify whether prior review findings were addressed
  • Post findings

Summary

One new bug found (website navbar omission). Three findings from the prior review remain unaddressed. The overall implementation is solid — the parser, graph construction, and RDS pattern are correct — but these four issues should be resolved before merging.


Issues

1. (NEW) concept-map.qmd missing from _quarto-website.yml navbar — _quarto-website.yml:92-125

The chapter is added to project.render at line 38, but not to the Appendices dropdown in the website.navbar section (lines 92-125). Every other chapter in the render list has a corresponding navbar entry:

# Line 122-125 — all present; concept-map is absent
- text: "Exam Formula Sheet"
  href: chapters/exam-formula-sheet.qmd
- text: "Package Versions"
  href: chapters/package-versions.qmd
# concept-map.qmd is missing here

The page will be rendered but unreachable from the website navbar. The book build (_quarto-book.yml) is correct.

Fix: Add to the Appendices menu in _quarto-website.yml:

- text: "Concept Map"
  href: chapters/concept-map.qmd

Fix this →


2. (Prior review, unfixed) Diagram labels show raw callout IDs — concept-map.qmd:90

aes(label = ifelse(n_desc >= 3, name, "")),

name is the igraph vertex name — the raw callout ID (e.g., thm-prob-to-odds). The extraction script already stores the human-readable heading text in V(core)$title. The labeled hubs will show machine identifiers instead of readable titles.

Fix: Use title instead of name:

aes(label = ifelse(n_desc >= 3, title, "")),

Fix this →


3. (Prior review, unfixed) do.call(rbind, nodes) crash when list is empty — callout-graph.R:125

nodes <- do.call(rbind, nodes)   # line 125 — no guard

The edges list correctly got a guard on lines 127-128, but nodes did not. do.call(rbind, list()) returns NULL, and graph_from_data_frame(..., vertices = NULL, ...) will fail with an opaque error. This matters when running the script in a clean or test environment with no matching .qmd files.

Fix:

nodes <- if (length(nodes)) do.call(rbind, nodes) else
  data.frame(id = character(), type = character(),
             file = character(),
             stringsAsFactors = FALSE)

Fix this →


4. (Prior review, unfixed) Proof-detection regex false-positive — callout-graph.R:79

is_proof <- grepl("proof|solution", content, ignore.case = TRUE)

content is the attribute string after stripping :::+, so {#disproof-theorem} or {#approval-process} would match and incorrectly inherit the preceding callout as the proof's owner.

Fix: Anchor to word boundaries:

is_proof <- grepl("\\b(proof|solution)\\b", content, ignore.case = TRUE)

{.proof} and {.solution} still match; {#disproof-foo} does not.

Fix this →


Observations (no action required)

subcomponent() deprecated in igraph 2.xcallout-graph.R:142, 149
The locked version is igraph 2.3.0. subcomponent() was deprecated in igraph 2.0.0 and will emit deprecation warnings when the script runs. Not a blocker (the computation is correct), but the modern replacement is bfs(ig, v, unreachable = FALSE)$order for vertex traversal.

Isolated-node prose is vacuously falseconcept-map.qmd:61-62
"Results with no detected dependency links … appear in @sec-descendants-table if they have descendants." A node with no edges cannot have descendants, so this clause is always false — isolates never appear in the table. The sentence could be trimmed to just "Results with no detected dependency links are omitted from the diagram."

distances() vs neighbors() for direct edgescallout-graph.R:148
distances(ig, v, mode = "out")[1, ] == 1 is correct but computes all-pairs distances to find direct neighbours. names(neighbors(ig, v, mode = "out")) is simpler for this purpose. No practical impact since the script runs offline.


What looks good: The edges empty-list guard at line 127-128, the stack-based parser handling nested divs and 4-colon fences, the adjacency rule for proof attribution, consistent edge direction semantics, set.seed(204), and the code-fold: true / div-format figure and table conventions — all correct and following project conventions.

@d-morrison

Copy link
Copy Markdown
Owner

@claude add the new chapter to the dropdown menu too; make a note to always do this when adding new chapters; maybe add a skill

@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor Author

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

@d-morrison

Copy link
Copy Markdown
Owner

@claude and make sure to show all the div titles, and use those titles as the nodes (maybe put boxes around them) instead of drawing circles with offset labels

@github-actions github-actions Bot requested a review from d-morrison June 4, 2026 00:14
@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor Author

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

@github-actions github-actions Bot requested a review from d-morrison June 6, 2026 00:52
@github-actions github-actions Bot removed the request for review from d-morrison June 9, 2026 07:48
@claude

claude Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

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


Code Review: PR #873 — Concept Map Appendix

  • Gather context
  • Read data-raw/callout-graph.R
  • Read chapters/concept-map.qmd
  • Read _quarto-book.yml / _quarto-website.yml changes
  • Read .claude/commands/new-chapter.md
  • Check status of all prior review findings
  • Post findings

Summary

All blocking bugs from prior rounds are resolved. All user requests from the last several comment cycles are correctly implemented. Two low-priority items remain open from earlier observations (never flagged as required fixes), and one minor new observation about new-chapter.md.


Resolved since last round ✓

Prior finding Fixed in
do.call(rbind, nodes) crash when list is empty (callout-graph.R:127) 566a09c
\index{} stripping + str_trim() in extraction script (callout-graph.R:61) 52e8329, fa7db3f
\index{} stripping + trimws() at render time (concept-map.qmd:36) 5f53a29, fa7db3f
Proof-detection regex false-positive (callout-graph.R:81) 52e8329
Diagram labels show raw IDs not titles (concept-map.qmd) e29bc1b
Min font size below 8pt (concept-map.qmd:122) fa7db3f — now range = c(4.0, 8.0) (4.0 mm ≈ 11 pt)
Dead whitespace in sugiyama layout fa7db3f — switched to "stress" layout
Direct-descendants column missing from table 13c85b2
Node label font not scaling by descendant count 13c85b2
Concept Map missing from website navbar d1ea852

Remaining observations (no blocking issues)

1. subcomponent() deprecated in igraph 2.x — callout-graph.R:191, 199

# line 191
function(v) length(subcomponent(ig, v, mode = "out")) - 1L,
# line 199
  reach  <- setdiff(subcomponent(ig, v, mode = "out")$name, v)

subcomponent() was deprecated in igraph 2.0.0 (locked version is 2.3.0) and emits a deprecation warning each time the script runs. The computation is correct. The modern replacement is bfs(ig, v, unreachable = FALSE)$order for vertex traversal; or subgraph_from_edges() + V() if staying close to the existing pattern. Low urgency — runs offline only.

Fix this →

2. new-chapter.md doesn't mention regenerating callout-graph.rds.claude/commands/new-chapter.md

Step 2 now correctly covers adding to the website navbar, but doesn't mention that adding callout divs in a new chapter requires re-running Rscript data-raw/callout-graph.R. The regeneration reminder exists in concept-map.qmd prose, but the workflow command is where a contributor (or @claude) would look when scaffolding a new chapter.

Suggested addition to step 2 in new-chapter.md:

If the chapter contains `def`/`thm`/`lem`/`cor`/`prp` callout divs, re-run
`Rscript data-raw/callout-graph.R` to refresh `inst/extdata/callout-graph.rds`.

Fix this →

3. Double-stripping of \index{} at render time — concept-map.qmd:36

cg$nodes$title <- trimws(gsub("\\\\index\\{[^}]*\\}", "", cg$nodes$title))

Since the extraction script (callout-graph.R:61) now strips \index{} before saving to the RDS, this render-time strip is redundant. It's harmless — graceful fallback for any RDS file built before the fix — but can be removed once the RDS is regenerated.


What looks good overall

  • graphlayouts (needed for layout = "stress") is confirmed present in renv.lock
  • Stress layout with geom_label_repel(max.overlaps = Inf, force = 3) is the right approach for even distribution without overlap ✓
  • scale_size_continuous(range = c(4.0, 8.0)) — minimum 4.0 mm ≈ 11 pt, well above the 8 pt floor ✓
  • HTML/revealjs-only figure with unless-format blocks; separate when-format fallback for PDF and docx ✓
  • concept-map.qmd placed in appendices in both _quarto-book.yml and _quarto-website.yml navbar ✓
  • new-chapter.md step 2 now reminds about navbar additions ✓
  • Table columns: Result (title), Type, Direct descendants, Total descendants — clean and no unresolved @ref links ✓
  • edges empty-list guard at line 132-133 ✓; nodes guard at line 127-130 ✓
  • set.seed(204) before create_layout() + seed = 204 in geom_label_repel both in place ✓

@d-morrison

Copy link
Copy Markdown
Owner

@claude there's still a ton of blank space; spread out the nodes

@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

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

@github-actions github-actions Bot requested a review from d-morrison June 9, 2026 07:51
@github-actions github-actions Bot removed the request for review from d-morrison June 18, 2026 17:03
@claude

claude Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

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


Code Review: PR #873 — Concept Map Appendix

  • Gather context and prior review history
  • Read data-raw/callout-graph.R (current state)
  • Read chapters/concept-map.qmd (current state)
  • Read _quarto-book.yml / _quarto-website.yml changes
  • Read .claude/commands/new-chapter.md
  • Check status of all prior review findings
  • Post findings

Summary

The latest commit (e049fb2) switched from sugiyama to FR layout with axis normalisation to address the "dead white space" complaint. The implementation is reasonable. All prior blocking bugs remain resolved. Two low-priority observations from earlier rounds remain open, and one minor new fragility was introduced by the normalisation.


Issues

1. norm_axis crashes with division-by-zero on a degenerate layout — concept-map.qmd:91-93

norm_axis <- function(x) (x - min(x)) / diff(range(x)) * 2 - 1
layout$x <- norm_axis(layout$x)
layout$y <- norm_axis(layout$y)

diff(range(x)) is zero when all nodes collapse to the same x (or y) coordinate. FR layout can do this for very sparse or path-shaped graphs. The result is NaN throughout, which causes ggraph to silently place all nodes at NA — the figure renders as an empty box with no error message. This won't hit the current 65-node graph, but if the script is regenerated after removing most edges the failure is invisible.

Fix: Guard before applying:

norm_axis <- function(x) {
  r <- diff(range(x))
  if (r == 0) return(x)
  (x - min(x)) / r * 2 - 1
}

Fix this →


2. (Prior review ×5, still unaddressed) subcomponent() deprecated in igraph 2.x — callout-graph.R:192, 199

# line 192
function(v) length(subcomponent(ig, v, mode = "out")) - 1L,
# line 199
reach <- setdiff(subcomponent(ig, v, mode = "out")$name, v)

The locked version is igraph 2.3.0. subcomponent() was deprecated in igraph 2.0.0 and emits a deprecation warning each time the script runs. Computation is correct; the modern replacement is:

bfs(ig, v, unreachable = FALSE)$order

Low urgency since this runs offline, but the warning will appear every time Rscript data-raw/callout-graph.R is run.

Fix this →


3. (Prior review ×2, still unaddressed) new-chapter.md missing callout-graph.rds regeneration step — .claude/commands/new-chapter.md

Step 2 now correctly covers adding to the website navbar and the render list, but the workflow doesn't mention that adding def/thm/lem/cor/prp callout divs in a new chapter requires re-running Rscript data-raw/callout-graph.R. A contributor following the skill command would miss this.

Suggested addition after step 4:

5. If the chapter contains `def`/`thm`/`lem`/`cor`/`prp` callout divs,
   re-run `Rscript data-raw/callout-graph.R` to refresh
   `inst/extdata/callout-graph.rds` and keep the concept map current.

Fix this →


Observations (no action required)

Redundant \index{} stripping at render timeconcept-map.qmd:36
callout-graph.R:61 now strips \index{} before saving to the RDS, so the render-time strip in concept-map.qmd:36 is redundant. It's a harmless defensive fallback for RDS files built before the fix, and can be cleaned up after the next RDS regeneration.

Aspect ratio distortion from independent axis normalisationconcept-map.qmd:91-93
Normalising x and y independently (each to [-1, 1]) stretches whichever axis was more compressed in the raw FR output. For a graph with natural y-clustering, this can amplify vertical spread in a way that looks unnatural. An alternative is to normalise by the same factor on both axes (preserving aspect ratio while eliminating asymmetric margins). This is a visual judgement call, not a correctness issue.


Resolved Since Last Round ✓

User request / Prior finding Fixed in
Dead whitespace / spread nodes e049fb2 — FR layout + axis normalisation
All four prior-round blocking bugs ✓ committed earlier

What Looks Good

  • FR layout + axis normalisation is a pragmatic solution: FR's global repulsion spreads isolated clusters, and the normalisation eliminates margin whitespace. niter = 2000 gives better convergence than the default 500.
  • nodes empty-list guard at lines 127-130 ✓; edges guard at lines 132-133 ✓
  • Proof-detection uses \\b(proof|solution)\\b with word boundaries ✓
  • \index{} stripped + str_trim() applied at extraction time ✓
  • _quarto-website.yml: chapter in render: list and Appendices navbar ✓
  • _quarto-book.yml: chapter in appendices list ✓
  • new-chapter.md step 2 navbar reminder ✓
  • HTML/revealjs-only figure via nested unless-format blocks; separate fallback prose for PDF and docx ✓
  • Count-only descendants table (no unresolved @ref links) ✓
  • set.seed(204) before create_layout() + seed = 204 in geom_label_repel

@github-actions github-actions Bot requested a review from d-morrison June 18, 2026 17:06
@d-morrison

Copy link
Copy Markdown
Owner

@claude address reviews and make font size bigger

@github-actions

Copy link
Copy Markdown
Contributor Author

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

claude Bot and others added 3 commits June 18, 2026 20:47
Replace the two subcomponent() calls (deprecated in igraph 2.x) with
bfs(..., mode = "out", unreachable = FALSE)$order equivalents. No change
in computed results; removes deprecation warnings when the script runs.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- norm_axis: guard against diff(range(x)) == 0 (all nodes collapse to the
  same coordinate in a sparse graph) to prevent silent NaN positions and
  an empty figure.
- scale_size_continuous: widen font-size range from c(4.0, 8.0) to
  c(8.0, 16.0) to make node labels more readable.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When a new chapter contains def/thm/lem/cor/prp callout divs, contributors
need to re-run Rscript data-raw/callout-graph.R to keep the concept map
current. Add this as an explicit step 5 in the new-chapter skill.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions github-actions Bot requested review from d-morrison and removed request for d-morrison June 18, 2026 20:51
claude added 2 commits June 19, 2026 08:03
The committed callout-graph.rds was stale: def-expectation had acquired
\index{} commands in its heading after the last regeneration, and the
dependency graph had grown (new content merged from main). Re-run
data-raw/callout-graph.R to refresh it (434 results, 115 links). With the
.rds back in sync, the extraction script already strips \index{} from
titles, so the duplicate render-time strip in concept-map.qmd is redundant
and removed.

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

Copy link
Copy Markdown
Owner

Addressed all three findings from the latest review (main merged in):

  • Address (already done, confirmed) — subcomponent()bfs() in data-raw/callout-graph.R (commit 68e1149). No subcomponent() remains; bfs(…, unreachable = FALSE)$order produces sane, varied descendant counts (n_desc 0–39, n_direct 0–7; top hub def-probability 7 direct / 39 total).
  • Address (already done, confirmed) — new-chapter.md step 5 now reminds to re-run callout-graph.R (commit cb82496).
  • Address (commit 9a75c85) — removed the redundant render-time \index{} strip in concept-map.qmd:36. Important: the committed .rds was staledef-expectation had acquired \index{expectation} \index{expected value} in its heading after the last regeneration, so the render-time strip was still doing real work and removing it blindly would have regressed that title to raw LaTeX. I re-ran data-raw/callout-graph.R to refresh the artifact (now 434 results / 115 links, all titles \index{}-free), after which the script's own extraction-time strip makes the render-time copy genuinely redundant. The regenerated .rds also picks up dependency-graph drift from content merged via main.

Checks: quarto render chapters/concept-map.qmd --to html ✓, lintr::lint("chapters/concept-map.qmd") ✓, spelling::spell_check_package() ✓.

@claude please re-review.


Generated by Claude Code

@github-actions github-actions Bot removed the request for review from d-morrison June 19, 2026 08:08
@github-actions

Copy link
Copy Markdown
Contributor Author

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

@claude

claude Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

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


Code Review: PR #873 — Concept Map Appendix

  • Gather context and prior review history
  • Read data-raw/callout-graph.R (current state)
  • Read chapters/concept-map.qmd (current state)
  • Read .claude/commands/new-chapter.md
  • Verify the three addressed findings from last round
  • Check for new issues
  • Post findings

Summary

All three findings from the previous round are confirmed resolved, as is every blocking issue from prior rounds. No new bugs found. Two long-standing non-blocking observations remain.


Resolved in this round ✓

Finding Commit Confirmed
subcomponent() deprecated → replaced with bfs() 68e1149 callout-graph.R:192,199bfs(ig, v, mode = "out", unreachable = FALSE)$order
new-chapter.md step 5: re-run callout-graph.R cb82496 git diff origin/main...HEAD confirms step 5 is in committed state ✓
Redundant render-time \index{} strip removed 9a75c85 concept-map.qmd:35 — only readRDS(); no post-load stripping ✓

All Prior Blocking Issues — Resolved ✓

Finding Fixed in
do.call(rbind, nodes) crash when list empty 566a09ccallout-graph.R:127-130
Proof-detection regex (proof|solution) false-positive 52e8329 — word boundaries \\b(proof|solution)\\b
\index{} in extracted titles + str_trim() 52e8329, fa7db3fcallout-graph.R:61
Diagram labels showing raw IDs not titles e29bc1bgeom_label_repel(aes(label = title, ...))
norm_axis division-by-zero on degenerate layout 489a72bconcept-map.qmd:90-94 guard ✓
Website navbar missing Concept Map entry d1ea852_quarto-website.yml:127-128
Font size below 8pt fa7db3f, 489a72brange = c(8.0, 16.0) (8mm ≈ 22.7pt min) ✓
Direct descendants column in table 13c85b2concept-map.qmd:176
new-chapter.md navbar reminder dd97e4a — step 2 expanded ✓

Remaining Observations (no action required)

distances() for direct neighbours — callout-graph.R:198

direct <- setdiff(names(which(distances(ig, v, mode = "out")[1, ] == 1)), v)

distances() computes the full shortest-path matrix from v to all vertices, then filters to distance 1. neighbors(ig, v, mode = "out") would give the same result in a single pass. No practical impact since this runs offline; noted for completeness.

Independent axis normalisation preserves no aspect ratio — concept-map.qmd:95-96

Normalising x and y independently (each to [-1, 1]) stretches whichever axis was naturally compressed in the FR output. This is the same visual-judgement observation from the prior round; the current layout trades aspect-ratio fidelity for full canvas use. Fine as-is.


What Looks Good Overall

  • callout-graph.R parser: stack-based, handles 4-colon fences and nested non-callout divs ✓
  • Edge direction from = referenced, to = referencing → outgoing edges reach descendants ✓
  • bfs() descendant counting at lines 192, 199 — correct mode and unreachable-exclusion ✓
  • edges guard at callout-graph.R:132-133; nodes guard at 127-130
  • concept-map.qmd placed in both _quarto-book.yml appendices and _quarto-website.yml navbar and render list ✓
  • HTML/revealjs-only figure via nested unless-format blocks; PDF and docx fallback prose ✓
  • Count-only descendants table — no unresolved @ref links ✓
  • set.seed(204) before create_layout() + seed = 204 in geom_label_repel
  • \index{} stripped at extraction time in callout-graph.R:61; redundant render-time strip correctly removed ✓
  • All three packages (igraph, ggraph, ggrepel) confirmed present in renv.lock

The PR is ready to merge.

@github-actions github-actions Bot requested a review from d-morrison June 19, 2026 08:12
@github-actions

Copy link
Copy Markdown
Contributor Author

Working tree is clean and no additional @claude requests arrived. My own read of the current files confirms the prior session's conclusion. Here is my review comment:


Code Review: PR #873 — Concept Map Appendix

Re-review of the three addressed findings from the triggering comment — all confirmed resolved.

Verified

Finding Location Status
subcomponent()bfs() callout-graph.R:192, 199 bfs(ig, v, mode = "out", unreachable = FALSE)$order — correct mode, - 1L to exclude start vertex
new-chapter.md step 5 re-run reminder .claude/commands/new-chapter.md ✅ Confirmed in prior session (commit cb82496)
Render-time \index{} strip removed after RDS refresh concept-map.qmd ✅ No post-load strip; extraction-time strip in callout-graph.R:61 handles it; RDS regenerated at 434 results / 115 links

Everything else checked and still good

  • norm_axis division-by-zero guard ✓
  • geom_label_repel uses title (not raw id), size = n_desc, range = c(8.0, 16.0) (min 8pt) ✓
  • Direct descendants column in @tbl-descendants, sorted by total descendants ✓
  • HTML/RevealJS-only figure; PDF and docx fallback prose with link ✓
  • concept-map.qmd in _quarto-book.yml appendices section ✓
  • _quarto-website.yml navbar entry present ✓
  • set.seed(204) in layout + seed = 204 in geom_label_repel for reproducibility ✓

The two non-blocking observations from prior rounds (distances() vs neighbors() for direct edges; independent-axis normalisation) remain unchanged — neither is a correctness issue.

The PR is ready to merge.

— posted by @claude post-step from workflow run

@github-actions

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

Built to branch gh-pages at 2026-06-19 08:28 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.

2 participants