diff --git a/devdocs/tutorial-evaluation/README.md b/devdocs/tutorial-evaluation/README.md new file mode 100644 index 0000000..f508748 --- /dev/null +++ b/devdocs/tutorial-evaluation/README.md @@ -0,0 +1,125 @@ +# Tutorial evaluation report + +A critical evaluation of the `heormodel` website tutorials, read from the perspective of the target reader: a health economics and outcomes research (HEOR) modeler at the graduate level or beyond, who builds cost-effectiveness models and writes code but is not a software engineer. The evaluation covers each tutorial on its own and the sequence the tutorials form together. + +Each tutorial has its own evaluation file (`eval-.md`) in this folder, written against the repository writing style guide (`devdocs/guidance/writing_style.md`) and the tutorial-quality criteria it sets. This page synthesizes those evaluations and adds the cross-cutting findings that only appear when the tutorials are read as a set: where a concept is introduced relative to where it is first used, whether the pages link together in a coherent path, and where they repeat or contradict each other. + +## How the evaluation was run + +Each tutorial was reviewed by a separate reviewer that read the writing style guide, the tutorials index, and the tutorial itself, checked the rendered output where it could, and rated the page on purpose, pedagogy, writing, correctness, and sequencing. The cross-cutting analysis below comes from one reading of all twenty pages in order, including the site homepage and the replication gallery. + +## Verdicts at a glance + +Ratings are Excellent, Good, Needs work, or Weak. This table is filled in as the per-tutorial evaluations land. + +| # | Tutorial | Section | Rating | +|---|---|---|---| +| 1 | Full pipeline | Getting started | Needs work | +| 2 | Bring your own outputs | Getting started | Good | +| 3 | Parameter inputs from data | Getting started | Good | +| 4 | Cohort state-transition model | Model engines | pending | +| 5 | Microsimulation engine | Model engines | pending | +| 6 | Markov vs microsimulation models | Model engines | pending | +| 7 | Discrete-event simulation engine | Model engines | pending | +| 8 | Compartmental transmission model | Model engines | pending | +| 9 | Deterministic sensitivity analysis | Sensitivity and VoI | pending | +| 10 | Value of information | Sensitivity and VoI | pending | +| 11 | Calibration workflow | Calibration | pending | +| 12 | Calibrating with ABC | Calibration | pending | +| 13 | Calibrating with simulation-based inference | Calibration | pending | +| 14 | Surrogate-accelerated calibration | Calibration | pending | +| 15 | Calibrating a stochastic microsimulation | Calibration | pending | +| 16 | Time-dependent cohort model | Replications | pending | +| 17 | Microsimulation replication | Replications | pending | +| 18 | Discrete-event simulation replication | Replications | pending | + +## The set is strong at the page level and weak at the seams + +Read one at a time, most of these tutorials are good. The prose follows the methods-appendix voice the style guide asks for, the mathematics of each estimand is stated before it is used, and results are confirmed against published numbers where a published number exists. The calibration sequence is the high point: four tutorials calibrate one shared model against one shared target with escalating methods, and each ends by naming the limitation the next one removes. + +The weaknesses are almost all at the seams between tutorials, not inside them. The forward links contradict the reading order, value-of-information results are interpreted six tutorials before value of information is taught, the getting-started section opens with data plumbing rather than a model, and the replication gallery promises an analysis that two of its four pages do not run. These are the findings that a page-by-page review cannot surface, and they are where the most useful editing is. + +## Cross-cutting finding 1: the forward links contradict the reading order + +Every tutorial ends with a "Next:" link, and the sidebar lists the tutorials in a fixed order, but the two disagree. A reader who navigates by the "Next:" links does not walk the sidebar order; the links appear to be left over from an earlier arrangement in which the full-pipeline tutorial came after the engines. + +The sidebar order is: full pipeline, bring your own outputs, parameter inputs, then the five engine tutorials, then deterministic sensitivity analysis and value of information, then the five calibration tutorials, then the replications. The "Next:" links instead run: + +- Full pipeline points to value of information, skipping eight tutorials. +- Bring your own outputs points to the cohort model, skipping parameter inputs. +- Parameter inputs points to deterministic sensitivity analysis, skipping every engine. +- The cohort model points to value of information. +- Discrete-event simulation points backward to the full pipeline, a getting-started page. +- The compartmental model, the last engine, has no "Next:" link at all, and nothing links to it. +- Deterministic sensitivity analysis points to the replication gallery, skipping value of information, the item directly below it. +- Value of information has no "Next:" link and ends on a plot. +- Calibration workflow points backward to parameter inputs, a getting-started page. + +Only the microsimulation-to-cohort-comparison-to-discrete-event stretch and the calibration arc link forward correctly. A reader following the links loops between the discrete-event, full-pipeline, and value-of-information pages and never reaches the compartmental model or the sensitivity pages in a natural order. This is the single highest-value, lowest-effort fix in the set: make every "Next:" link point to the next tutorial in the sidebar, give the compartmental model and value-of-information pages a forward link, and remove the backward links. + +## Cross-cutting finding 2: value of information is used before it is taught + +The expected value of perfect information (EVPI) and the per-parameter ranking by expected value of partial perfect information (EVPPI) appear, with interpreted numbers, in the full-pipeline tutorial (page 1), bring your own outputs (page 2), the cohort model (page 4), the microsimulation (page 5), discrete-event simulation (page 7), and the compartmental model (page 8). The net monetary benefit and EVPI equations are first written down in bring your own outputs, page 2. The dedicated value-of-information tutorial, which defines EVPI, EVPPI, expected value of sample information (EVSI), and expected net benefit of sampling, is page 10. + +The reader therefore meets six worked EVPI figures, each interpreted as if the quantity were already understood, before the page that defines it. The early tutorials treat "the expected value of perfect information is positive, so resolving the uncertainty is worth this much" as self-explanatory. For a reader who has not yet done a value-of-information analysis, it is not. Two fixes are compatible: add one or two sentences defining EVPI at its first appearance, or have the early tutorials state plainly that they are previewing a measure the value-of-information tutorial defines, and link to it, rather than interpreting the number in place. + +## Cross-cutting finding 3: the getting-started section opens with plumbing, not a model + +The first three tutorials are the full pipeline, bring your own outputs, and parameter inputs from data. The full-pipeline tutorial builds a two-branch decision tree through `Outcomes.from_wide`, not one of the package's four model engines. Bring your own outputs ingests a results table computed elsewhere. Parameter inputs ingests draw matrices and a posterior sample. The reader does not build a model with one of the engines until the cohort tutorial, page 4. + +Two of the first three tutorials are about getting data into and out of the analysis without building a model, and the first teaches on a synthetic decision tree. The style guide states that a tutorial should teach against a real model engine rather than a synthetic construct. A reader who came to the package to build cost-effectiveness models meets three pages of input and output plumbing before the first engine. This ordering was a deliberate choice recorded in the roadmap, so it is worth revisiting rather than simply an oversight, but from the target reader's point of view the lede is buried: the natural first tutorial is "build a cohort model and analyze it," with the bring-your-own-outputs and parameter-input pages positioned as the two ways to enter the workflow from the side once the reader has seen it run. + +## Cross-cutting finding 4: the homepage quickstart overlaps the full-pipeline tutorial + +The site homepage runs a complete three-state Markov cohort analysis: parameter sampling, `icer_table`, a cost-effectiveness plane, a cost-effectiveness acceptability curve, and an EVPI curve swept over the willingness-to-pay threshold. The full-pipeline tutorial, the first tutorial, runs a smaller analysis on a decision tree and interprets its results less fully. The homepage quickstart is a more complete pipeline than the page named "full pipeline." The two should be differentiated: either the full-pipeline tutorial should carry the analysis the homepage only previews, on a real engine, or it should be repositioned so it is not the reader's second encounter with the same material. + +## Cross-cutting finding 5: the replication gallery over-promises + +The replication gallery states that each of its four replications "first matches the source's deterministic results, then runs the same model through `heormodel.cea` and `heormodel.voi` to show how a probabilistic analysis extends it." That is true for the discrete-event replication and for the cohort replication, which is cross-listed under the engine tutorials. It is not true for the time-dependent cohort replication or the microsimulation replication: both stop at the deterministic base-case `icer_table` and run no probabilistic sensitivity analysis, no acceptability curve, and no value-of-information analysis. The gallery's own summary claim holds for two of its four entries. Either extend the time-dependent and microsimulation replications with the short probabilistic analysis the gallery promises, or soften the gallery text to describe what those two pages actually do. + +## Cross-cutting finding 6: the first engine tutorial is the heaviest + +The cohort tutorial is the reader's first real model engine, and it is also a sixteen-parameter published replication with rate-to-probability conversion, hazard ratios applied to rates, a Simpson within-cycle correction, and densely packed transition-matrix assignments. The microsimulation tutorial that follows uses simpler, plainly illustrative invented parameters. The entry engine is the most demanding page in the engine section, not the least. The published replication is valuable, but a reader meeting `MarkovModel` for the first time has to absorb the engine and a faithful reproduction of a real article at once. Consider whether the first engine tutorial should teach the engine on a small illustrative model and leave the full published replication to the gallery, where the cohort replication is already cross-listed. + +## Cross-cutting finding 7: the calibration workflow sits ahead of the method it uses + +The calibration section opens with the calibration-workflow tutorial, whose subject is mixing calibrated and literature parameters with `mix_draws` and recording their provenance. To do that it calls `abc_calibrate` and shows a posterior, but approximate Bayesian computation is not explained until the next tutorial. The workflow tutorial also uses a different disease model, a continuous-time generator matrix, from the shared three-state model that the following four calibration tutorials all use. The ABC tutorial then introduces itself as "the first of four calibration tutorials that share one disease model," which reads oddly when it is the second page in the section. The workflow tutorial is really an application of calibration, not an introduction to it. Placing it after the ABC tutorial, so the reader knows what a posterior and a calibration run are before being shown how to mix one into an analysis, would remove the forward dependency. + +## Cross-cutting finding 8: repetition and code density + +The four-state Sick-Sicker cohort model is defined almost verbatim in three tutorials: the cohort model, deterministic sensitivity analysis, and value of information. The repetition is partly justified, since each page must run on its own and the style guide endorses repeating the shared workflow, but the full twenty-five-line model function copied three times is more than the shared-workflow point requires. The dense transition-matrix assignments, which pack several matrix entries onto one line, are within the style guide's allowance for replicated-paper notation but are hard for a reader who is learning to build the model rather than read it. The discrete-event tutorial carries the steepest coding prerequisite in the set, since it requires fluency with the underlying discrete-event library; that page needs the most care to stay readable for a modeler who is not a software engineer. + +## Provisional recommendation on sequence + +The individual pages need light editing; the sequence needs a deliberate second pass. In priority order: + +1. Repair the forward navigation so the "Next:" links match the sidebar order, and give every page a forward link. This is small and removes the most visible defect. +2. Resolve where value of information is introduced, so the reader is not asked to interpret an EVPI before it is defined. +3. Reconsider the opening: lead the getting-started section with a real engine, and position the bring-your-own-outputs and parameter-input pages as side entrances to the workflow rather than the first thing the reader sees. +4. Align the replication gallery's promise with what its pages do. +5. Move the calibration workflow after the ABC tutorial. + +The per-tutorial files carry the page-level detail behind these findings. + +## Specific edits for the cross-cutting findings + +These are copy-ready changes for the findings above. Each per-tutorial file carries the same level of specificity for its own page. + +Finding 1, forward links. Set each tutorial's closing "Next:" link to the next page in the sidebar order, and add the two missing links: + +- `full-pipeline.qmd`: change the closing link so "Next:" points to `byo-outputs.qmd`, not `voi.qmd`. +- `byo-outputs.qmd`: point "Next:" to `parameter-inputs.qmd`, not `mdm-cohort.qmd`. +- `parameter-inputs.qmd`: point "Next:" to `mdm-cohort.qmd`, not `dsa.qmd`. +- `mdm-cohort.qmd`: point "Next:" to `microsim.qmd`, not `voi.qmd`. +- `des.qmd`: point "Next:" to `seir-vaccination.qmd`, not `full-pipeline.qmd`. +- `seir-vaccination.qmd`: add a closing line, "Next: [deterministic sensitivity analysis](dsa.qmd) asks which parameters move the result, before [value of information](voi.qmd) asks what resolving the remaining uncertainty is worth." +- `dsa.qmd`: point "Next:" to `voi.qmd`, not `replication-gallery.qmd`. +- `voi.qmd`: add a closing line, "Next: the [calibration workflow](calibration-workflow.qmd) fits model parameters to data and carries the fitted uncertainty into the same analysis." +- `calibration-workflow.qmd`: point "Next:" to `calibrate-abc.qmd`, not `parameter-inputs.qmd` (see finding 7). + +Finding 2, value of information used before it is taught. At the first EVPI figure in `full-pipeline.qmd` (the "Analyzing cost-effectiveness and value of information" section), add after the sentence that prints the EVPI: "The expected value of perfect information is the average gain from resolving all parameter uncertainty before deciding, an upper bound on what any study could be worth; the [value of information](voi.qmd) tutorial defines it and the related measures in full." Add the same one-clause pointer at the first EVPI in `byo-outputs.qmd`, `microsim.qmd`, `des.qmd`, and `seir-vaccination.qmd`, so each interprets its number against a named, linked definition rather than assuming it. + +Finding 4, replication gallery. In `replication-gallery.qmd`, the intro sentence "Each replication first matches the source's deterministic results, then runs the same model through `heormodel.cea` and `heormodel.voi` to show how a probabilistic analysis extends it" is true only for the cohort and discrete-event pages. Replace it with: "Each replication first matches the source's deterministic results. The cohort and discrete-event pages then run the same model through `heormodel.cea` and `heormodel.voi` to show how a probabilistic analysis extends it; the time-dependent and microsimulation pages stop at the deterministic match." The stronger fix is to add a short probabilistic sensitivity analysis to `mdm-cohort-timedep.qmd` and `mdm-microsim.qmd` so the original sentence holds; the per-tutorial files for those two give the exact code to add. + +Finding 7, calibration workflow placement. Move `calibration-workflow.qmd` to follow `calibrate-abc.qmd` in both `docs/_quarto.yml` (the sidebar `contents` list) and `docs/tutorials/index.qmd` (the `calibration` listing). Then `calibrate-abc.qmd`'s self-description, "the first of four calibration tutorials that share one disease model," becomes true, and the workflow page no longer calls `abc_calibrate` before approximate Bayesian computation is defined. + diff --git a/devdocs/tutorial-evaluation/eval-byo-outputs.md b/devdocs/tutorial-evaluation/eval-byo-outputs.md new file mode 100644 index 0000000..4ed34b0 --- /dev/null +++ b/devdocs/tutorial-evaluation/eval-byo-outputs.md @@ -0,0 +1,60 @@ +# Evaluation: Bring your own outputs (`byo-outputs.qmd`) + +**Slot:** Getting started (2 of 3) + +**Overall verdict:** A genuinely useful page for the audience it targets, a modeler whose costs and effects already exist in a spreadsheet or a legacy simulator, and its placement right after `full-pipeline` is the correct call: the reader has already seen what `Outcomes` and `run_psa` are for, so being told they can skip the model-building step lands as intended rather than as a mystery. The code runs cleanly and every numeric claim in the prose checks out against the actual computation. Its main shortfall is pedagogical follow-through: several sections state what a plot or record generically shows without reading the specific rendered numbers back to the reader, which is exactly the "show, do not describe" standard the style guide sets. **Rating: Good.** + +## Strengths + +- **Sequencing is right.** Coming after `full-pipeline` (which already builds a toy model and calls `icer_table`, `evpi`, and `evppi_ranking`), this page's value proposition, "you do not need a model engine to use the analysis layer", is legible. Placed first, it would leave the reader unsure what `Outcomes` is supposed to represent or why `icer_table` exists at all. +- **Verified correctness.** I ran the tutorial's own code (`N=2_000`, seed `20260704`) end to end. Every claim matches: all three interventions come back `ND` (non-dominated), the frontier ICERs land at approximately 20,971 and 38,074 so the willingness-to-pay threshold of 30,000 does sit between them as the text says, and `evppi_ranking` returns values sorted highest to lowest (`u_gain` first) exactly as claimed in "The ranking is sorted from the highest value downward" (line 101). +- **Good cross-referencing instead of repetition.** `full-pipeline.qmd` explicitly defers to this page ("[Bring your own outputs] covers the analysis functions in more detail; here the focus is the run itself"), and this page in turn signposts forward to `mdm-cohort.qmd` and sideways to `parameter-inputs.qmd` ("bring your own outputs is the analogue on the outcome side", quoted from `parameter-inputs.qmd`). This is the "link rather than repeat" discipline the style guide asks for, done correctly across three pages. +- **Named constants up top.** `WTP` and `N` are lifted to the top of the first code block with an inline comment explaining the choice of `WTP`: `WTP = 30_000.0 # sits between the two frontier ratios, so the decision is uncertain` (line 24). That is precisely the "why this threshold" content the guide asks for, verified against the actual output rather than asserted. +- **Headings are parallel and correctly named.** "Simulating an external results table", "Converting the table to the standard structure", "Ranking parameters by their value of information" are all gerund phrases naming what the section does, matching the guide's rule. +- **`as_outcomes` claim is accurate.** "`as_outcomes` accepts a `DataFrame` or a CSV path with `intervention`, `iteration`, `cost`, and effect columns, and returns `Outcomes`" (line 55) matches the function's actual signature and docstring in `src/heormodel/run/runner.py` exactly. + +## Weaknesses and gaps + +1. **Plots are described generically, not interpreted specifically.** The guide requires "a sentence or two after the output that interprets it: what the number or plot means for the decision, not a restatement of what was just computed" (writing_style.md, "What makes a tutorial good," item 3). Every plot in the "Plotting the cost-effectiveness plane, acceptability, and tornado" section gets only a *before*-the-code sentence describing what that chart type shows in general, and no *after*-the-code sentence reading off what this particular rendered figure actually shows: + - `plot_ce_plane(...)`: preceded by "the spread of each intervention's regions shows the uncertainty that the table's point estimates alone cannot" (line 105), and then the page moves straight to the acceptability curves with no sentence about what the New drug and Drug + monitoring regions actually look like relative to the 30,000 line. + - `plot_ceac(...)`: "making the crossover in the decision visible" (line 113) describes CEACs in general; it does not say where the crossover falls on this run's grid. + - `plot_tornado(...)`: "ranking each by how much it moves the incremental net benefit at the threshold" (line 119) is again a description of what a tornado plot does, not of which parameter actually tops this one. Given that `u_gain` already topped the `evppi_ranking` table two paragraphs earlier, one sentence tying the tornado back to that ("`u_gain` again dominates, consistent with the EVPPI ranking above") would cost nothing and would satisfy the guide's own standard: "a plot beats an adjective." + This is a real gap, not a nitpick: the style guide gives this exact package's chart types (CEAC) as its worked example of what a good, specific interpretation looks like, and this page does not clear that bar it sets for itself. + +2. **The final section ends with no interpretation at all.** After `capture_run` prints the run report (lines 130–135), the page moves directly to "Next: the [Markov cohort model]..." (line 138) with no sentence about what is actually in that printed record or why a reviewer would trust it. Every other section closes with at least one interpretive sentence; this one, the closing section of the tutorial, does not. + +3. **No target number stated up front.** The style guide is explicit: "Put a specific outcome in the title and on the first screen... the opening states the result they are working toward, the target number or figure, before the code that produces it." The opening paragraph (line 12) previews the four kinds of output (cost-effectiveness, value of information, plots, a reproducibility record) but no actual number, e.g., that the frontier will run all three interventions non-dominated with an EVPI near 550 per person at the 30,000 threshold. A reader has no way to sanity-check partway through that their run is heading somewhere sensible until the ICER table appears midway down the page. + +4. **The linked example script uses banned business jargon.** The page tells the reader to walk through `examples/byoo_example.py` "step by step" (line 12), but that script's own module docstring reads: "This script demonstrates **the adoption wedge** of `heormodel`" (examples/byoo_example.py, line 3). "Adoption wedge" is explicitly listed in the style guide as banned business jargon ("wedge, adoption wedge, go-to-market... describe product intervention, not a cost-effectiveness analysis"). The guide applies to "everything written in this repository," and a reader following the tutorial's own link will land on this line. It is outside the `.qmd` file itself but is a direct, load-bearing dependency of it. + +5. **Minor redundancy in framing.** The near-identical phrase "a spreadsheet export or a legacy simulator's output" (or a close variant) appears three times: in the front-matter description, in the opening paragraph (line 12), and again in the first section ("A real analysis would load this table from a spreadsheet or a legacy simulator's output," line 16). None of the three is wrong, but by the third repetition it reads as restating rather than adding. One of the three could be cut without losing anything, in keeping with "if a paragraph restates the previous one, delete it." + +## Writing-style issues (with quoted lines) + +Overall the prose is clean and largely compliant; I did not find em-dashes, exclamation marks, or any of the explicitly banned words (leverage, delve, seamless, comprehensive, robust, crucial, streamline) inside `byo-outputs.qmd` itself. The specific issues that do stand out: + +- **Acronym spelled out inconsistently across related quantities.** EVPI gets the full treatment: "The expected value of perfect information (EVPI) puts a monetary value on resolving every remaining source of uncertainty at once" (line 88). But CEAC and CEAF, the two other named quantities this same package computes and that the guide names as its own worked example ("`Cost-effectiveness acceptability curve (CEAC)`" appears verbatim in writing_style.md as the model heading), never get spelled out anywhere on the page. The prose only ever says "`ceac` and `ceaf` turn that uncertainty into acceptability curves" (line 74), using the lower-case function names as if they were the concept names. A reader who has not seen "cost-effectiveness acceptability curve" spelled out before is left to infer it from the code identifier. +- **Heading names the action, not the computed quantity, where the guide's own example says to do the opposite.** The section "Ranking parameters by their value of information" (line 82) computes EVPPI (expected value of partial perfect information) via `evppi_ranking`, but the heading paraphrases the action rather than naming the quantity. The guide's exact wording: "Naming the canonical quantity a section computes takes priority over parallelism... a plain 'Calculating the expected value of perfect information (EVPI)' beats an evocative paraphrase of its purpose." A heading such as "Expected value of partial perfect information (EVPPI) by parameter" would both name the quantity and spell out the acronym in the one place the page never does. +- **Section heading mixes three chart types under one label.** "Plotting the cost-effectiveness plane, acceptability, and tornado" (line 103) is a noun list rather than a single named action, unlike every other heading on the page. It is not wrong, but it is the one heading that reads as an inventory rather than a name. + +## Concept introduction and sequencing + +- **No forward reference problem.** Everything the page uses, `ParameterSet`, `Beta.from_mean_se`, `Normal`, `Gamma.from_mean_se`, correlated sampling, `SeedManager`, was introduced with at least a sentence of explanation in `full-pipeline.qmd` first. The one exception, the `correlation={("p_response", "u_gain"): 0.3}` argument (line 36), was only *mentioned* in `full-pipeline`'s prose ("a Spearman rank correlation can be imposed on any pair of parameters") without being shown in that page's code; this page is the first to actually use it. That is a small gap but not a serious one, since the prose sentence already primed the concept. +- **Reasonable overlap with `voi.qmd`, not a duplication.** This page gives the EVPI formula ($\text{EVPI} = \mathbb{E}_\theta[\max_d \text{NMB}_d(\theta)] - \max_d \mathbb{E}_\theta[\text{NMB}_d(\theta)]$, line 90) that `voi.qmd` never repeats (there it stays prose-only: "EVPI is the difference between a decision made with perfect information and one made with current information"). `voi.qmd` in turn gives the EVPPI formula that this page never repeats. The split is deliberate and not redundant, but it does mean a "getting started" tutorial is the first place a reader meets a $\max_d \mathbb{E}_\theta$ expression, ahead of the dedicated `dsa.qmd` and `voi.qmd` pages in the "Sensitivity & VoI" group. That is defensible, since the formula clarifies what the functions compute before the reader uses them repeatedly, but it does mean this page is doing more mathematical lifting than its "getting started" position might suggest to a reader skimming the table of contents. +- **Combines more than one workflow step, consistent with `full-pipeline`'s own pattern.** The page covers cost-effectiveness analysis, value-of-information ranking, three plot types, and a reproducibility record, four distinct topics in one tutorial. Taken alone this looks like it strains "teach one method per tutorial," but `full-pipeline.qmd` already establishes CEA + VoI as one bundled "standard workflow" rather than as two separate methods, and this page is consistent with that established pattern rather than introducing a new one. No change recommended here; flagged only because it is a real trade-off, not an oversight. +- **Placement relative to `parameter-inputs.qmd` is correct.** The two pages are explicit analogues, "bring your own outputs is the analogue on the outcome side" (from `parameter-inputs.qmd`), one substituting the outcome table, the other substituting the parameter draws. Putting the outcome-side substitution before the parameter-side one is a reasonable choice, since results computed elsewhere (this page) is the more common practitioner starting point than an externally exported draw matrix (the next page). + +## Prioritized recommendations + +**Major** + +1. Add one interpretive sentence after each plot that reads the actual rendered result, not the general chart type: where the CEAC crossover falls, which region dominates in the cost-effectiveness plane, and which parameter tops the tornado (tie it back to the EVPPI ranking already shown). This is the single highest-value fix; it is the concrete gap between this page and the standard the style guide sets for itself. +2. Add a closing sentence after the `capture_run` output stating what the printed record actually shows (e.g., which seed and iteration count a reviewer would see) before the "Next" link. +3. Fix the module docstring in `examples/byoo_example.py` to drop "the adoption wedge of `heormodel`" (banned business jargon per the style guide) in favor of a plain description, since the tutorial sends readers directly to that file. + +**Minor** + +4. Spell out "cost-effectiveness acceptability curve (CEAC)" and, if used again, "cost-effectiveness acceptability frontier (CEAF)" on first use in prose rather than only ever writing the lower-case function names `ceac`/`ceaf`. +5. Rename "Ranking parameters by their value of information" to name the computed quantity, e.g. "Expected value of partial perfect information (EVPPI) by parameter", matching the guide's own worked example for this exact package. +6. State a specific target number in the opening paragraph (for example, the EVPI value or that all three interventions land non-dominated) so the reader has something concrete to check their own run against partway through. +7. Cut one of the three near-identical repetitions of "a spreadsheet export or a legacy simulator's output" across the description, the opening paragraph, and the first section. diff --git a/devdocs/tutorial-evaluation/eval-calibrate-abc.md b/devdocs/tutorial-evaluation/eval-calibrate-abc.md new file mode 100644 index 0000000..bf6beb5 --- /dev/null +++ b/devdocs/tutorial-evaluation/eval-calibrate-abc.md @@ -0,0 +1,44 @@ +# Evaluation: Calibrating with ABC (`calibrate-abc.qmd`) + +**Slot:** Calibration (2 of 5) + +**Overall verdict:** This is the page that actually teaches approximate Bayesian computation (ABC): the mechanism (propose, accept within tolerance, anneal the tolerance), the finite-sample target, and the simulator-mirrors-measurement idea are all introduced before use and interpreted after, in correct, checkable prose. It is let down by one quantitative claim I could verify and found wrong by close to an order of magnitude, by ABC's own hyperparameters sitting inline instead of in named constants, and by a sequencing problem that is not this page's fault but that this page cannot avoid: `calibration-workflow.qmd`, one page earlier in the site's own order, already calls `abc_calibrate` and interprets a posterior with no equivalent explanation. Rating: Good. + +## Strengths + +- The opening does exactly what the style guide asks: state the goal, name the method, say what it does, link out. "This tutorial shows how to calibrate an unknown model parameter to observed data using [approximate Bayesian computation (ABC)](https://en.wikipedia.org/wiki/Approximate_Bayesian_computation), a method that infers parameters by simulating from the model and keeping the parameter sets whose output matches the data." I opened the link; it resolves to the real Wikipedia article. +- The mechanism is stated, not just named, and stated correctly: propose from the prior, run the model, accept within a tolerance, $\rho\big(S(\theta),\, S_\text{obs}\big) \le \varepsilon$, then anneal $\varepsilon$ down. This is the one thing `calibration-workflow.qmd` skipped, and here it gets three paragraphs and an equation before any code runs. +- The finite-sample framing is well built and reused correctly throughout: "The target is not the prevalence itself but a measurement of it," followed by a printed table of true prevalence, observed survey, and standard error. The simulator-mirrors-measurement idea, easy to get backwards, is stated plainly: "The simulator mirrors the measurement, returning a fresh sample of the model's prevalence rather than the prevalence itself, so ABC recovers the parameters given a noisy reading and carries the sampling error into the posterior." +- Decision inputs for the model and target are properly lifted: `STATES`, `N_CYCLES`, `TARGET_CYCLES`, `BOUNDS`, `TRUTH`, `SURVEY_SIZE`, `SURVEY_SEED` all sit as named constants at the top of their blocks, exactly the pattern the guide asks for. +- The example is a real model engine, the package's own `CohortSpec`/`MarkovModel`, not a synthetic construct built to check a closed form. Generating the target from a known truth is stated as a deliberate design choice ("gives the calibration a right answer to recover"), which is the guide's own permitted pattern, not undisclosed scaffolding. +- Headings are parallel gerunds ("Specifying," "Measuring," "Running"), no em-dashes, no exclamation marks, no banned vocabulary, and the acronym ABC is spelled out once and reused correctly afterward. +- The "Next:" link to `calibrate-sbi.qmd` is correct and matches the sidebar order, part of what an earlier cross-cutting review already identified as the best-linked stretch of the site. + +## Weaknesses and gaps + +- **The run-count claim does not hold up when checked.** The page says: "Reaching this posterior took several thousand model evaluations, because every accepted parameter set in every population is a model run and most proposals are rejected." I ran the companion script, `examples/calibrate_abc.py`, twice with the tutorial's exact settings (`population_size=400`, `max_populations=15`, the same `min_epsilon` formula, `seed=1`). It reports 24,519 model runs the first time and 23,321 the second. That is tens of thousands, not "several thousand," off by roughly an order of magnitude from the phrase used. The qmd's own code blocks never count or print a run total, so a reader cannot check this claim from the page itself, and when checked against the one place it can be checked, the companion script, it is wrong. +- **The ABC run is not actually fully seeded, and the page does not say so.** `abc_calibrate(..., seed=1)` reads as pinning the whole run, but the function's own docstring says otherwise: "seed: Seed for the weighted-to-equal resampling step. (The ABC run itself uses pyabc's internal randomness.)" My two runs above, same code, same `seed=1`, produced different population counts of model runs and slightly different final epsilons (0.0094 vs 0.0096) and posterior standard deviations. A reader who reruns this page will not see the same "final epsilon" number the page states it printed, and nothing in the prose warns of that, which is a plain limitation the style guide asks to state rather than leave implicit. +- **ABC's own hyperparameters are not lifted to constants**, unlike the survey and model inputs. `population_size=400`, `max_populations=15`, `n_posterior=3_000`, and `seed=1` sit inline inside the `abc_calibrate` call, and `sim_rng = np.random.default_rng(2024)` is an inline seed too. These are exactly the values a reader adapting this tutorial to a slower model would need to change first (fewer particles, fewer populations), so leaving them inline, while `SURVEY_SIZE`/`SURVEY_SEED`/`BOUNDS`/`TRUTH` are correctly hoisted a few lines above, is an inconsistency the guide's own rule ("lift the decision inputs... to named constants") is meant to prevent. `calibration-workflow.qmd` has the identical gap with the same call. +- **The "within about a standard error" claim is accurate for one parameter and optimistic for the other.** Across my two runs, `p_HS`'s posterior mean sits about 0.02 to 0.07 SD from truth, but `p_SD`'s sits about 1.5 SD from truth both times (0.1405 vs 0.15, SD 0.0060; 0.1409 vs 0.15, SD 0.0059). "Within about a standard error" is a fair summary for `p_HS` alone; for `p_SD` it understates the gap. This is a minor point next to the run-count claim, but it is the same kind of issue: a summary phrase standing in for a number the page could have printed and did not. + +## Writing-style issues (with quoted lines) + +The prose itself is clean; the issues below are about a claim's accuracy, not word choice. + +- "Reaching this posterior took several thousand model evaluations, because every accepted parameter set in every population is a model run and most proposals are rejected." This sentence follows the guide's rule to prefer a number over an adjective, which makes it worse, not better, that the number is wrong: a precise-sounding figure that does not survive a rerun is less trustworthy than a vaguer one would have been. +- No em-dashes, no exclamation marks, no rhetorical questions, and no instances of the banned vocabulary (leverage, delve, seamless, comprehensive, robust, crucial, streamline) turned up on a full read. +- The guide's first-person-plural register ("we specify," "we start by") is absent throughout; the page is written in an impersonal declarative voice instead ("The model is a three-state cohort..."). That voice is still within a methods-appendix register and matches the sibling `calibration-workflow.qmd`, so this reads as a section-wide convention rather than a defect specific to this page, but it is worth noting since the guide names "we" as the default for tutorials. + +## Concept introduction and sequencing + +The page earns its "first of four" self-description on its own terms: it is the first calibration tutorial to actually explain ABC, and `calibrate-sbi.qmd`, read alongside it, confirms the shared setup ("The model and the target are the same as in the ABC tutorial") down to the same `STATES`, `TARGET_CYCLES`, `BACKGROUND_MORTALITY`, and `CALIBRATED` names. The four-tutorial arc this page opens is internally consistent and the best cross-linked part of the site. + +The problem is where "first of four" sits relative to the section as a whole. `docs/tutorials/index.qmd` places `calibration-workflow.qmd` immediately before this page, and that page already calls `abc_calibrate` and interprets a posterior with only one clause of explanation, "runs approximate Bayesian computation (sequential Monte Carlo)... and returns an iteration-indexed posterior draw matrix," no tolerance, no acceptance rule, no link. A reader who follows the site's listed order meets ABC's output a full tutorial before meeting the mechanism this page teaches. This page cannot fix that by itself: its own text never even acknowledges `calibration-workflow.qmd` came first, so a reader arriving here in order gets the real explanation with no signal that it was overdue. The fix belongs at the index and workflow-tutorial level (already recommended in the `calibration-workflow.qmd` evaluation, move that tutorial to follow this one), not in this page's text. + +## Prioritized recommendations (major first, then minor) + +1. Fix or remove the run-count claim. Either print an actual count from the qmd's own code (mirroring `model_runs["count"]` in `examples/calibrate_abc.py`) and state the real figure, or soften "several thousand" to a description that does not commit to a number the page cannot show, such as "tens of thousands of model evaluations, most of them rejected." +2. State the non-determinism of the ABC-SMC run plainly. One sentence noting that `seed` fixes only the posterior resampling, not pyabc's own sampling, so the number of populations, final epsilon, and run count will vary slightly between runs, would turn a silent reproducibility gap into a stated limitation, which is what the guide asks for. +3. Lift `population_size`, `max_populations`, `n_posterior`, `seed`, and the simulator's `sim_rng` seed to named constants alongside `BOUNDS`, `TRUTH`, `SURVEY_SIZE`, and `SURVEY_SEED`, so a reader adapting the tutorial to a slower model edits one block rather than the `abc_calibrate` call itself. +4. Soften "within about a standard error of the truth" to acknowledge that one parameter (`p_SD`) sits further out, or report the ratio directly rather than a summary phrase. +5. Minor: a one-clause forward acknowledgment that `calibration-workflow.qmd` used `abc_calibrate` already, pointing the reader to this page for the explanation they skipped, would help a reader following the site's listed order even before that page is moved. diff --git a/devdocs/tutorial-evaluation/eval-calibration-workflow.md b/devdocs/tutorial-evaluation/eval-calibration-workflow.md new file mode 100644 index 0000000..7a08358 --- /dev/null +++ b/devdocs/tutorial-evaluation/eval-calibration-workflow.md @@ -0,0 +1,43 @@ +# Evaluation: Calibration workflow (`calibration-workflow.qmd`) + +**Slot:** Calibration (1 of 5) + +**Overall verdict:** Sentence by sentence this is careful, correct writing: every interpretive claim checks out against a live run, `mix_draws` and `capture_run` are introduced before use and interpreted after, and the headings and prose follow the style guide closely. But the page is placed wrong. It calls `abc_calibrate` and interprets a posterior before approximate Bayesian computation (ABC) is ever defined, its own disease model is a bespoke continuous-time construct that none of the other four calibration tutorials share, and its "Next" link sends the reader backward into "Getting started" instead of forward into the section it opens. Rating: Needs work. + +## Strengths + +- The opening states the goal in the form the guide asks for, not an inventory of calls: "This tutorial shows how to mix two parameter sources in one analysis: some parameters are calibrated to observed data, and the rest come from the literature." It also states the extra dependency in the first paragraph, as required: "Calibration needs the `calibration` extra: `uv pip install 'heormodel[calibration]'`." +- Every claim I could check checks out. Re-running the page's code (same seeds, same settings) gives posterior means `onset 0.0202`, `progression 0.0516`, matching "close to the onset and progression rates that generated the targets, 0.02 and 0.05." The correlation comes out to 0.962, matching "strongly correlated." The EVPPI ranking comes out `rr_progression 721.7 > c_treat 302.9 > u_sick 294.7 > c_sick 121.1 > onset 78.8 > progression 56.1`, matching both "`rr_progression` leads the ranking" and "the calibrated rates rank at the bottom." +- `mix_draws` and provenance capture are model examples of introduce-then-interpret. Before use: "It resamples whole rows within each source, so the posterior's joint correlation is preserved, and resamples sources independently, so no correlation is introduced across them," which matches the function's own docstring almost verbatim. After use: "One matrix now holds both sources, so the decision model reads every parameter, calibrated and literature, from a single set of columns." +- Three independent random streams (ABC, literature sampling, mixing) are drawn from one `SeedManager` via `seeds.spawn(3)` rather than three ad hoc seeds, which is the right reproducibility pattern for a page that stitches sources together. +- Headings are parallel gerunds throughout ("Specifying," "Calibrating," "Sampling," "Mixing," "Running... and analyzing," "Recording"), no em-dashes or exclamation marks appear, and the closed-form paragraph gives a real, verifiable reason for its own approach, "so no cycle loop is needed," rather than an invented one. + +## Weaknesses and gaps + +- **`abc_calibrate` is used before ABC is defined for this reader.** The only explanation offered is one clause: "`abc_calibrate` runs approximate Bayesian computation (sequential Monte Carlo) against the observed calibration targets and returns an iteration-indexed posterior draw matrix." There is no mention of a prior, a tolerance, an accepted-simulation rule, or a link to a further reading, the treatment `calibrate-abc.qmd` gives the same method three paragraphs and an external link for. A reader following the site's own listed order (`calibration-workflow` before `calibrate-abc`, per `docs/tutorials/index.qmd`) meets the method's output and its interpretation with no explanation of the mechanism producing it. +- **The disease model is a second, unexplained departure from the rest of the section.** This page's model is a continuous-time generator matrix solved by `(rI-Q)^{-1}`; `calibrate-abc.qmd`, `calibrate-sbi.qmd`, `surrogate-calibration.qmd`, and `calibrate-microsim.qmd` share a discrete-cycle cohort model built from the package's own `CohortSpec`/`MarkovModel`. `calibrate-abc.qmd` describes itself as "the first of four calibration tutorials that share one disease model, so the posteriors can be compared across methods," which by construction excludes this page. Placed first, this page teaches a third thing (continuous-time Markov theory and matrix inversion) that none of the other calibration tutorials build on and that is never linked to further reading, on top of the mixing workflow and an unexplained ABC call. +- **Decision inputs are not fully lifted.** `DISCOUNT`, `MORTALITY`, `START_AGE`, `N`, and `WTP` are named constants, but the prior bounds (`Uniform(0.005, 0.04)`, `Uniform(0.01, 0.1)`), the observed targets (`{"prev_age50": 0.147, "prev_age70": 0.283}`), and the ABC settings (`population_size=200`, `max_populations=6`) sit inline inside the `abc_calibrate` call. `calibrate-abc.qmd` lifts its analogous values into named `BOUNDS` and `TRUTH` dictionaries at the top of the block; this page does not follow that pattern even though it is teaching the same call. +- **The ABC acronym is never formally introduced.** The page spells out "approximate Bayesian computation" once, but the parenthetical that follows is the algorithm variant, "(sequential Monte Carlo)," not the acronym. "ABC" then appears unintroduced in the provenance code, `{"ABC posterior" for name in posterior.columns}`, so a reader who has not already read `calibrate-abc.qmd` sees the abbreviation used before it is ever defined on this page. + +## Writing-style issues (with quoted lines) + +No banned vocabulary, no em-dashes, no exclamation marks, and no anthropomorphizing turned up on a full read. The two issues worth flagging are both about what is left unexplained rather than about word choice: + +- "`abc_calibrate` runs approximate Bayesian computation (sequential Monte Carlo) against the observed calibration targets and returns an iteration-indexed posterior draw matrix." This is accurate but is the entire explanation of the method the section is named for ("Calibrating the rates"). Compare `calibrate-abc.qmd`'s opening, which spells out the acronym, states the inverse-problem framing, gives the acceptance rule as an inequality, and links to a Wikipedia article, all before running any code. +- "Discounted state occupancy has a closed form: for a chain with generator matrix $Q$ and discount rate $r$, the expected discounted years spent in each state, starting from the initial distribution $e_0$, are $e_0 (rI - Q)^{-1}$, so no cycle loop is needed." This sentence does state a reason ("so no cycle loop is needed"), which satisfies the guide's verifiable-why rule, but it introduces a second unfamiliar piece of machinery, a continuous-time Markov generator matrix, with no link out for a reader whose own models are the package's discrete-cycle cohort models, as every other tutorial in the site uses. + +## Concept introduction and sequencing + +The prompt's premise is confirmed by the files themselves. `docs/tutorials/index.qmd` lists this page first in the Calibration section: "The workflow tutorial comes first, the next three compare inference methods on one shared model." But `calibrate-abc.qmd`'s own description field reads "the first of four calibration tutorials that share one disease model," and its opening paragraph repeats it: "It is the first of four calibration tutorials that share one disease model, so the posteriors can be compared across methods." Both statements cannot be true of the same section at once: either `calibration-workflow.qmd` is first (per the index) or `calibrate-abc.qmd` is first (per its own text), and the page under review is the one that introduces a method it has not taught yet, so it is the one out of place. + +The page's own navigation compounds this. Its lead paragraph says "It builds on the [full pipeline tutorial]," which is a defensible prerequisite (the ICER table, EVPI, and EVPPI ranking calls are unchanged from `full-pipeline.qmd`), but its closing line reads "Next: [parameter inputs from data](parameter-inputs.qmd) covers the base-case, imported, and posterior draw matrices that feed the same analysis." `parameter-inputs.qmd` is the third page in "Getting started," two full sections earlier in the site's own reading order. A reader who has just finished the first page of calibration is sent backward out of the section entirely, rather than forward to `calibrate-abc.qmd`, the page that both the index and `calibrate-abc.qmd`'s own text expect to come next. + +Moving this page to follow `calibrate-abc.qmd` (and ideally the full four-tutorial ABC/SBI/surrogate/microsim comparison, since it depends on none of their shared model but does depend on the reader already knowing what ABC is) would fix both problems at once: `abc_calibrate` would be used only after it has been taught, and the "first of four" self-description in `calibrate-abc.qmd` would finally match its actual position. The alternative, keeping this page first, requires giving ABC a real introduction here (an accept/reject rule, a link) and either adopting the shared model or explicitly stating why this page departs from it. + +## Prioritized recommendations (major first, then minor) + +1. Move `calibration-workflow.qmd` to follow `calibrate-abc.qmd` in `docs/tutorials/index.qmd`'s Calibration listing (and adjust `calibrate-abc.qmd`'s "first of four" description if a fifth, workflow-focused tutorial now precedes it in name only). This resolves the direct contradiction between the index's stated order and `calibrate-abc.qmd`'s own self-description, and removes the pedagogical gap of calling `abc_calibrate` before ABC is defined. +2. Fix the "Next" pointer. "Next: parameter inputs from data" sends the reader backward into "Getting started"; it should point forward to `calibrate-abc.qmd` (or whichever tutorial precedes it after the reorder in recommendation 1). +3. If this page keeps its own continuous-time, generator-matrix disease model rather than adopting the four-tutorial shared cohort model, add one sentence linking to further reading on continuous-time Markov chains, the same treatment the page already gives `mix_draws` and provenance capture, and the treatment `calibrate-abc.qmd` gives ABC. +4. Lift the prior bounds, the observed targets, and `population_size`/`max_populations` to named constants, matching the `BOUNDS`/`TRUTH` pattern already used one tutorial away in `calibrate-abc.qmd`. +5. Minor: introduce the ABC acronym formally, "approximate Bayesian computation (ABC)," on its own first use, since the abbreviation is later used unexplained in the provenance code's source labels. diff --git a/devdocs/tutorial-evaluation/eval-des.md b/devdocs/tutorial-evaluation/eval-des.md new file mode 100644 index 0000000..cf1afdd --- /dev/null +++ b/devdocs/tutorial-evaluation/eval-des.md @@ -0,0 +1,58 @@ +# Evaluation: Discrete-event simulation engine (`des.qmd`) + +**Slot:** Model engines (4 of 5) + +**Overall verdict:** The engine-specific reasoning is sound and every number I reran matched the prose exactly, but the page drops the reader into SimPy's generator-based programming style, `yield`, a resource requested as a context manager, and an event race between the queue and the horizon, without explaining any of it, for an audience the style guide defines as explicitly not software engineers. Combined with a "Next" link that points backward into a getting-started tutorial instead of forward to the last engine tutorial, and the expected value of perfect information (EVPI), an acronym this page never spells out even once, this is **Needs work**, not **Good**. + +## Strengths + +- **The opening earns the "when to reach for this" test.** "the model type to use when a shared, scarce resource makes patients queue, so that one patient's outcome depends on the others in the system, a coupling neither a cohort nor an independent microsimulation model can represent" gives a real, checkable criterion (a shared resource, not just "individual-level detail") and the model that follows demonstrates exactly that coupling through a single specialist `Resource`. +- **Every number I reran matched the prose.** I ran `examples/des.py` directly: the mean queue wait is 27.3 days under standard capacity and 1.1 days under expanded capacity, consistent with "The single specialist leaves patients waiting weeks; the second clears the queue to about a day." `service_time` leads the expected value of partial perfect information ranking by more than an order of magnitude over the next parameter (511.9 versus 29.9), consistent with "Service time leads the value-of-information ranking." Expanded capacity costs more and gains quality-adjusted life-years, consistent with the prose interpretation. +- **The decision-lever-versus-branching handoff is clean, not repeated.** `microsim.qmd` already teaches the distinction in depth and forward-links explicitly ("The discrete-event tutorial does this for clinic capacity"), and this page cashes that reference in one sentence ("Capacity is a parameter the model already reads, not a flag standing in for which arm is which") rather than re-deriving the rule. This is the guide's "link rather than repeat" principle done correctly. +- **The shared-workflow language is reused correctly.** "From here nothing is engine-specific: the same `icer_table`, `evpi`, and `evppi_ranking` calls used for a cohort or microsimulation model apply to this queueing model's `Outcomes`" is exactly the repetition the guide calls a feature: it tells the reader the pattern transfers. +- **No em-dash, exclamation mark, or banned-vocabulary violation anywhere on the page**, and the three section headings ("Modeling the queueing process," "Configuring once, evaluating on draws," "Analyzing cost-effectiveness, value of information, and the queue") are gerund-form and parallel with each other and with the sibling engine tutorials. +- **`toolkit.accrue_over` gets a real explanation at first use**: "integrates and discounts a flow over an absolute interval, so billing the queueing time a patient just endured is one call once the specialist is granted" states both the mechanism and why one call suffices. + +## Weaknesses and gaps + +1. **The SimPy mechanics this whole model depends on are never explained, for a reader the style guide says is not a software engineer.** The `clinic` function is a Python generator (`yield env.timeout(...)`), the specialist is requested as a context manager (`with toolkit.request("specialist") as slot:`), and the horizon race uses SimPy's event-condition operator (`slot | env.timeout(...)`) followed by a membership test (`served = slot in result`) to determine which event won. None of `yield`, the context-manager pattern, or the race operator is named or explained in prose anywhere on the page. The single sentence that follows the code, "A patient still waiting at the horizon is never seen: the horizon arrives before the specialist, and only the waiting segment is billed," states the *effect* of the race but never the *mechanism* (`slot | env.timeout(...)` returns whichever event fires first; `served` checks whether that was the specialist). A reader unfamiliar with SimPy's generator-based process model has no way to read this code, only to copy it. +2. **`toolkit.state("waiting")` is used with zero explanation anywhere on the page.** It appears in the first code block and is never mentioned in the surrounding prose. Its purpose, marking a state so that the event log `queue_waits` later reads can identify what a patient was doing, only becomes even partly inferable three sections later when `collect="events"` and `queue_waits` are introduced, and even then the connection back to `toolkit.state` is never made explicit. +3. **No link to SimPy's own documentation.** The guide's treatment of an unfamiliar method elsewhere on the site ("Name the method, say in a sentence or two what it does and why it is used here, and link to a reputable external page for a reader who wants the full treatment") is not applied to SimPy, even though this page introduces an entire simulation paradigm (event-driven, generator-based processes) that nothing else in the tutorial sequence uses. There is also no reference page in `docs/` describing `toolkit`'s methods (`request`, `state`, `accrue_over`, `accrue_cost`, `rng`, `horizon`), so this page is the reader's only source, which raises the cost of leaving these calls unexplained. +4. **EVPI is never spelled out on this page.** The style guide requires spelling out every acronym on first use per page. `mdm-cohort.qmd`, `microsim.qmd`, and `seir-vaccination.qmd` each write "expected value of perfect information" (`seir-vaccination.qmd` even parenthesizes the acronym) at least once; `des.qmd` never does, using only the lowercase function name `evpi` in prose and code. The expected value of partial perfect information that `evppi_ranking` computes is likewise never named in full anywhere on the page. +5. **The EVPI value itself is never interpreted, unlike its siblings.** `mdm-cohort.qmd` explains why its EVPI is positive ("because Intervention AB... is the best choice in some draws but not others"); `microsim.qmd` ties its EVPI to threshold placement ("the decision is genuinely uncertain"). `des.qmd` prints "EVPI at WTP 30,000: 770.7" and moves straight to interpreting the expected value of partial perfect information ranking without saying what 770.7 means for this decision. +6. **The willingness-to-pay threshold of 30,000 is asserted with no justification in the tutorial prose.** `examples/des.py`, the script this page says it walks through, carries the reasoning as a code comment: `WTP = 30_000.0 # the two staffing levels are about equally likely to be optimal here`. That is exactly the kind of verifiable "why" sentence the guide asks the tutorial to keep, and it was dropped when the value was copied into the page. +7. **The tutorial's own PSA sample size (128) does not match the example script it says it walks through (256).** `examples/des.py` sets `N_PSA = 256`, but the tutorial's code block draws `parameters.sample(128, seed=seeds.generator())`. I reran the tutorial's exact code: the incremental cost-effectiveness ratio comes out to 18,690 and EVPI to 783.6, both different from the values `examples/des.py` itself prints (17,576 and 770.7). The qualitative claims in the prose (service time leads the ranking, capacity costs more and gains quality-adjusted life-years) hold under both sample sizes, but a reader comparing the two side by side would find numbers that do not match, despite the page's claim to walk through the linked script step by step. +8. **`n_individuals=30` against `ARRIVAL_RATE=15` patients per year exhausts the arrival stream well before the four-year horizon, and nothing on the page says so.** I checked directly: with these settings, the last of the 30 patients arrives at about year 2.55, so roughly the last third of the declared "four-year horizon" sees no new arrivals at all (already-treated patients keep accruing follow-up to the horizon, but the queue stops receiving new patients). The prose describes patients arriving "as a Poisson process at rate `ARRIVAL_RATE`" over "a four-year horizon" without noting that the population size bounds how much of that horizon actually fills with arrivals, an interaction a reader adapting this pattern to a longer horizon or higher volume needs to understand and currently cannot learn from this page. + +## Writing-style issues (with quoted lines) + +- No em-dash, exclamation mark, or filler-vocabulary (leverage, comprehensive, robust, streamline, and so on) violation was found. +- Acronym handling is the one clear violation: "EVPI" and "evpi" appear in code and in an f-string the code prints, but "expected value of perfect information" never appears in the page's prose, unlike the three sibling engine tutorials (see weakness 4). +- "so billing the queueing time a patient just endured is one call once the specialist is granted" reads correctly under the guide's carve-out for interpreting a number already shown, since no figure is being requoted here, this is plain description of a mechanism, not a violation. +- "The single specialist leaves patients waiting weeks; the second clears the queue to about a day" is a correct use of the same carve-out: the exact figures (27.3 and 1.1 days) are printed in the code output directly above, so the sentence may state plainly what they mean rather than repeat them, and "weeks" is numerically right for 27.3 days (3.9 weeks). +- Headings are gerund-form and parallel: "Modeling the queueing process," "Configuring once, evaluating on draws," "Analyzing cost-effectiveness, value of information, and the queue." No seam here. +- "the coupling neither a cohort nor an independent microsimulation model can represent" and "a coupling neither a cohort nor an independent microsimulation model can represent" (opening paragraph) echoes `index.qmd`'s own summary of the model-engines group almost verbatim, a deliberate and appropriate consistency, not a redundancy problem, since the two pages serve different readers (a scanning index versus a reader already on the page). + +## Concept introduction and sequencing + +- As the fourth engine tutorial, `des.qmd` correctly assumes and reuses what `microsim.qmd` teaches: "building the model once and running it on draws, and accruing discounted costs and effects over time" is named explicitly in the opening paragraph as inherited rather than retaught, and the decision-lever-versus-branching handoff (strength above) does the same for that specific concept. This part of the sequencing is done well. +- **The "Next" link is wrong.** The page ends: "Next: the [full pipeline tutorial](full-pipeline.qmd) walks through the whole workflow, from `ParameterSet` sampling through `run_psa` to the analysis functions." `full-pipeline.qmd` is a getting-started tutorial that logically precedes even `mdm-cohort.qmd`, the first model-engine tutorial; a reader who follows "Next" from `des.qmd` is sent backward past three tutorials they have already completed. `markov-vs-microsim.qmd`, the tutorial immediately before this one, correctly points forward: "Next: the discrete-event simulation engine adds a scarce, shared resource that couples entities together, a dependence neither the cohort nor the independent microsimulation carries." `des.qmd` should end the same way, pointing to `seir-vaccination.qmd`, the fifth and last model-engine tutorial in `index.qmd`'s declared order. +- **That forward link would currently lead to a dead end anyway.** `seir-vaccination.qmd` (model engines 5 of 5) has no "Next" line at all; it ends on a summary sentence about the ordinary differential equation engine with no pointer into the "Sensitivity & VoI" group that `index.qmd` places right after it. Fixing `des.qmd`'s link alone leaves the chain broken one page later; both pages need the fix for a reader to be able to follow "Next" through the whole model-engines group and into value-of-information. +- **Value-of-information functions are used here before `voi.qmd` formally defines them, but this is a sequence-wide pattern, not something `des.qmd` introduces.** `evppi_ranking` is already used in `full-pipeline.qmd` (the very first tutorial in the entire sequence) and in `microsim.qmd` before this page. `mdm-cohort.qmd` is the only model-engine tutorial that both spells out "expected value of perfect information" and forward-links to `voi.qmd`; none of `microsim.qmd`, `markov-vs-microsim.qmd`, `des.qmd`, or `seir-vaccination.qmd` repeats that link. On this page specifically, the absence is compounded by weakness 4 (EVPI never spelled out) and weakness 5 (its value never interpreted), so a reader arriving here without having retained `mdm-cohort.qmd`'s explanation gets no definition and no pointer to one. +- The notebook `docs/_notebooks/des.ipynb` has no `execution_count` and no output on any cell, confirmed by inspecting the notebook JSON: it has never been rendered. I verified the numbers independently by running `examples/des.py` and the tutorial's own code directly (see weaknesses 2 and 7), so the prose's claims are correct, but the guide's mandatory render-and-check step ("Run every executable doc and confirm the prose matches the output before committing") has not been completed for this page. + +## Prioritized recommendations + +**Major** + +1. Add prose that explains the SimPy mechanics the model depends on, at minimum: what the generator/`yield` pattern means for the `clinic` function, what `toolkit.request("specialist")` returns and why it is used with `with ... as slot:`, and what `slot | env.timeout(...)` and `served = slot in result` do (a race between two events, resolved by checking which one is present in the result). Link to SimPy's own documentation for the reader who wants the full treatment, matching how the guide asks unfamiliar methods elsewhere to be handled. +2. Explain `toolkit.state("waiting")` at its first use, including its role in the event log that `queue_waits` reads later, rather than leaving it unexplained until a partial inference becomes possible three sections on. +3. Fix the "Next" link to point to `seir-vaccination.qmd` instead of `full-pipeline.qmd`, and add a "Next" line to `seir-vaccination.qmd` pointing into the sensitivity-and-value-of-information group (`dsa.qmd`), so the model-engines chain is traversable start to finish. +4. Spell out "expected value of perfect information" at least once on this page, and add one sentence interpreting the EVPI value (770.7) for the decision, matching the treatment `mdm-cohort.qmd` and `microsim.qmd` give their own EVPI outputs. +5. Render `docs/_notebooks/des.ipynb` and confirm the printed output matches the prose, per the mandatory render-and-check step. + +**Minor** + +6. Match the tutorial's PSA sample size (currently 128) to the example script's (256), or explain the difference, so a reader comparing the two does not find mismatched incremental cost-effectiveness ratios and EVPI values for what the page calls the same analysis. +7. State the reasoning behind the willingness-to-pay threshold of 30,000, already written as a code comment in `examples/des.py` ("the two staffing levels are about equally likely to be optimal here"), instead of dropping it when the value moves into the tutorial. +8. Add a sentence noting the relationship between `n_individuals`, `ARRIVAL_RATE`, and `HORIZON`: at the values used here, the arrival stream is exhausted by about year 2.55, well inside the four-year horizon, which a reader scaling the example to a longer horizon or higher volume needs to account for. diff --git a/devdocs/tutorial-evaluation/eval-dsa.md b/devdocs/tutorial-evaluation/eval-dsa.md new file mode 100644 index 0000000..2723677 --- /dev/null +++ b/devdocs/tutorial-evaluation/eval-dsa.md @@ -0,0 +1,52 @@ +# Evaluation: Deterministic sensitivity analysis (`dsa.qmd`) + +**Slot:** Sensitivity and value of information (1 of 2) + +**Overall verdict:** The page opens with a correct, useful distinction between deterministic and probabilistic sensitivity analysis, and I independently reran every code block: the one-way sweep, the tornado ranking, and the grid all reproduce exactly the numbers and orderings the prose claims. It loses ground on a real domain-correctness problem (the one-at-a-time range applies a blanket plus-or-minus-20% to utility parameters that are bounded at 1, pushing two of them above their valid range), a heatmap section that never shows a heatmap, and a "Next" link that skips `voi.qmd`, the very next item in its own sidebar section, to point two sections ahead. Rating: **Good**. + +## Strengths + +- **The opening does the one job a sensitivity-analysis tutorial most needs to do.** "Probabilistic sensitivity analysis answers how uncertain a decision is; deterministic sensitivity analysis (DSA) instead answers which parameters move the result, and by how much" states the PSA-versus-DSA distinction in one sentence, in the terms a reader needs to decide whether this is the right page, exactly as the style guide asks ("This tutorial shows how to..." rather than an inventory of function calls). +- **Every numeric and ranking claim I checked is correct.** I installed the package with `uv` and reran the three code blocks directly. The one-way sweep gives 65,162 / 15,256 / -34,650 at `c_trtB` = 8,000 / 13,000 / 18,000, falling as claimed. The tornado's top two parameters by span are `u_trtA` (180,952) and `hr_S1S2_trtB` (156,306), exactly the pair the prose names as moving the result most, with a clear gap to the third-ranked `u_S1` (118,100). The grid's center cell (`c_trtA`=12,000, `c_trtB`=13,000) is 15,256, identical to the one-way sweep's base value, confirming "the grid center reproduces the one-way sweep's base value." +- **Concepts are introduced before use and interpreted after.** Incremental net monetary benefit gets its formula, $\text{INMB} = \lambda\,\Delta E - \Delta C$, and a plain-language reading ("positive exactly when the intervention is cost-effective at the threshold $\lambda$") before the first code block that computes it; each of the three designs gets a sentence of mechanism before its code and a sentence of interpretation after its output. +- **The willingness-to-pay threshold is a named constant**, `WTP = 100_000.0`, used consistently across all three designs rather than repeated as a bare literal, an improvement over `mdm-cohort.qmd`, which hardcodes 100,000 inside the `evpi` call. +- **Clean on the mechanical rules.** No em-dashes, no exclamation marks, sentence-case and grammatically parallel gerund headings ("Running a one-way sweep," "Building a one-at-a-time tornado," "Sweeping a two-way grid"), `DSA` spelled out on first use, and none of the banned filler words. +- **The closing paragraph ties the deterministic and probabilistic framings back together**: "Where the probabilistic analysis integrates over the joint distribution, the deterministic analysis holds everything else fixed and reads one axis at a time" is a correct, checkable summary, not a restatement. + +## Weaknesses and gaps + +1. **The one-at-a-time range violates a hard parameter bound for two utilities.** `ranges = {name: (0.8 * base[name], 1.2 * base[name]) for name in base.index}` applies plus-or-minus-20% uniformly to every parameter, including `u_H` (base 1.0, utility of the Healthy state) and `u_trtA` (base 0.95, utility of the treated Sick state). Both utilities are bounded above by 1 by definition, but the sweep pushes them to 1.2 and 1.14, values with no meaning as a quality-adjusted life-year weight. I confirmed this from the actual computed tornado table: `u_H`'s "high" scenario and the top-ranked `u_trtA` scenario both use these out-of-range values. Nothing in the code or prose catches or flags this. This is exactly the kind of domain error a HEOR reader would notice immediately, and it undercuts the tornado's headline result, since `u_trtA` is named as the single most influential parameter partly on the strength of an invalid 1.14 upper scenario. The package's own probabilistic parameterization for the same model (`Beta(300, 15)` for `u_trtA` in `mdm-cohort.qmd`) respects the [0, 1] bound; the deterministic tutorial's blanket rule does not. +2. **The "heatmap" section does not produce a heatmap.** The page's own description promises "read them as tornado and heatmap reports," and the tornado section delivers a real plot via `plot_tornado`. The grid section instead prints `heatmap_data(...).round(0)`, a plain numeric table with row and column labels, no color-coded plot. `examples/dsa.py`, the script this page links to, does build an actual `imshow` heatmap (`examples/dsa.py:158-166`) and saves it to `heatmap_dsa.png`; the tutorial does not reproduce that step or explain why it stops short of it. A reader who came for a "heatmap report" gets a small matrix of numbers instead. +3. **The claim "the state costs move it least" is not verifiable from what the page shows.** The tornado plot is built from `plot_tornado(td.head(6))`, the six largest-span parameters only. The interpreting sentence about the state costs (`c_H`, `c_S1`, `c_S2`) moving the result least describes rows that never appear in the rendered output. I confirmed the claim is true from the full table (`c_S1` and `c_H` are in fact the two smallest spans of all sixteen parameters, and `c_S2` is fourth-smallest), but the style guide's rule that a "why" sentence should follow from what the reader can check applies to correctness of display, not just correctness of fact: the reader has no way to confirm this sentence from the page as rendered. +4. **The willingness-to-pay constant is not near the top of the first code block.** The guide's rule is to lift decision inputs "to named constants near the top of the first code block." `WTP = 100_000.0` is the last line of a 34-line block, after the model function and engine construction. `voi.qmd`, which reuses this exact model, defines `WTP = 100_000.0` immediately after the imports, before the model function. `dsa.qmd` is the outlier against its own sibling page here. +5. **No stated reason for redefining the full model function.** The base-case section reintroduces the entire 14-line `model()` function from `mdm-cohort.qmd` verbatim, which is a reasonable and expected pattern (the style guide explicitly endorses reusing this shape across tutorials, and the package's own convention in `voi.qmd` does the same). But `voi.qmd` states the reason explicitly: "the code below reconstructs both so this page runs on its own." `dsa.qmd` never states why the model is repeated rather than imported or linked, leaving the reader to infer it is for runnability rather than being told. +6. **The one-way sweep's "crossing zero near its base value" is a loose description.** The swept values are exactly linear in `c_trtB` (confirmed: slope -9.98 per dollar across both intervals), so the true zero crossing is at about 14,529, not at the base value of 13,000. It sits inside the swept window and closer to base than to the high end, but "near its base value" slightly overstates the precision; "between the base and high scenarios" would be exact rather than approximate. + +## Writing-style issues (with quoted lines) + +- No punctuation or vocabulary violations found: no em-dashes, no exclamation marks, no banned words (leverage, delve, comprehensive, robust, streamline, and the rest). +- "`one_way` varies a single parameter across the listed values, holding the rest at base" and the equivalent sentences for `one_at_a_time` and `grid` are correctly plain, mechanism-first descriptions, not narration of the code line by line. +- The transition-matrix code repeats the single-letter names the project's own `P`/`p` example in CLAUDE.md flags as the thing to avoid: `P = np.zeros((4, 4))` and `def model(p, intervention):`. This is inherited unchanged from `mdm-cohort.qmd`; since that source page already has the same issue flagged, this tutorial simply propagates it rather than introducing it fresh, but it is present here too and a reader meeting the pattern for the first time in this tutorial (someone who read the sensitivity tutorials before the model-engine ones) has no better cue to decode `P` and `p` than in the source page. +- Headings are grammatically parallel across the page ("Running a one-way sweep," "Building a one-at-a-time tornado," "Sweeping a two-way grid"), meeting the guide's parallel-heading rule cleanly. +- The tornado concept itself (why a horizontal bar chart sorted by span looks like a "tornado," what the shape communicates) is never stated in a sentence; the page assumes the reader already knows the visual convention. For a HEOR audience this is a defensible assumption (tornado diagrams are close to universal in the field), so this is a minor, not a major, gap. + +## Concept introduction and sequencing + +- As the first sensitivity-and-value-of-information tutorial, `dsa.qmd` builds cleanly on `mdm-cohort.qmd`: it reuses the same states, interventions, and parameter names, links to that page for the model's derivation, and does not re-teach `run_psa` or the cost-effectiveness functions, consistent with "so a deterministic analysis reuses everything a probabilistic one does." +- **The "Next" link skips `voi.qmd`.** The page ends: "Next: the [replication gallery](replication-gallery.qmd) collects the published models these engines reproduce." Per `docs/tutorials/index.qmd`, the sensitivity-and-value-of-information section lists `dsa.qmd` then `voi.qmd`, and the replication gallery belongs to a separate section that comes after the entire calibration group. A reader following the "Next" link from `dsa.qmd` is routed past `voi.qmd` and past all five calibration tutorials in one jump. This compounds in the other direction too: `voi.qmd`'s own opening paragraph links back only to `mdm-cohort.qmd`, never to `dsa.qmd`, so the two sibling pages in this section have no link between them in either direction, only the shared sidebar grouping. +- The section's own index blurb, "Deterministic sensitivity analysis asks which parameters move the result; value-of-information analysis asks what resolving the remaining uncertainty is worth," previews a DSA-versus-VoI contrast that `dsa.qmd` itself never states; the tutorial's own contrast is DSA-versus-PSA. Both framings are correct and non-conflicting, but nothing in `dsa.qmd` hands the reader forward to the VoI framing the index promises, reinforcing the missing-link problem above. + +## Prioritized recommendations + +**Major** + +1. Fix the one-at-a-time range so it respects each parameter's valid domain, at minimum capping utility parameters (`u_H`, `u_trtA`, `u_S1`, `u_S2`) below 1, or use a smaller percentage or an absolute range for utilities near the ceiling rather than a blanket plus-or-minus-20% multiplier applied to every parameter regardless of type. +2. Fix the "Next" link to point to `voi.qmd`, the actual next item in the sensitivity-and-value-of-information section, and add a corresponding back-link from `voi.qmd` to `dsa.qmd` so the pair is connected in both directions. +3. Either add an actual heatmap plot for the grid section (a short `imshow` block, matching what `examples/dsa.py` already does) or change the page's description and the section's framing so it stops promising a "heatmap report" it does not render. + +**Minor** + +4. Move `WTP = 100_000.0` to just after the imports, before the model function, matching the placement already used correctly in `voi.qmd`. +5. Add one clause stating why the model function is redefined here rather than imported, matching `voi.qmd`'s "so this page runs on its own." +6. Either show the full tornado table (or at least the bottom rows) before the "the state costs move it least" sentence, or rephrase the sentence to reference only what `td.head(6)` displays. +7. Replace "crossing zero near its base value" with the more precise "between the base and high scenarios," since the true zero crossing (about 14,529) sits closer to base than to the high end but is not at or very near base itself. diff --git a/devdocs/tutorial-evaluation/eval-full-pipeline.md b/devdocs/tutorial-evaluation/eval-full-pipeline.md new file mode 100644 index 0000000..efd4d2b --- /dev/null +++ b/devdocs/tutorial-evaluation/eval-full-pipeline.md @@ -0,0 +1,58 @@ +# Evaluation: Full pipeline (`full-pipeline.qmd`) + +**Slot:** Getting started (1 of 3) + +**Overall verdict:** The mechanics are sound and the prose is clean by the letter of the style guide (parallel gerund headings, no banned words, no em-dashes, acronyms spelled out), but the tutorial stops short of doing the one thing a first tutorial most needs to do: connect its own numbers into a decision. It prints an ICER of about 121,204 per QALY, then a willingness-to-pay threshold of 50,000, then an EVPI of 47.03, without ever saying that the ICER is more than double the threshold, which is exactly why the EVPI is so low. That connection is the payoff of the whole workflow, and a reader has to compute it themselves. Rating: **Needs work**. + +## Strengths + +- Mechanically correct. I ran the tutorial's code directly against the installed package: the draw matrix, `outcomes.summary()`, `icer_table`, `evpi`, `evppi_ranking`, and the `running_means` plot all execute and match the shapes the prose describes (ICER 121,203.865, EVPI 47.03 at willingness-to-pay 50,000, `rr_treat` the dominant driver in the EVPPI ranking at 9.75). The running-means plot genuinely does flatten out by iteration 4,000 for both interventions, so the one interpretive claim tied to a plot ("flat traces at the right edge suggest the iteration count is adequate") is true of this run. +- Headings are grammatically parallel gerunds throughout ("Specifying and sampling parameters", "Writing the model as a function", "Running the probabilistic sensitivity analysis", "Analyzing cost-effectiveness and value of information", "Checking convergence with running means"), matching the guide's requirement. +- No em-dashes, no exclamation marks, no banned filler words (leverage, comprehensive, robust, streamline, and so on all absent), acronyms (ICER, QALY, EVPI) spelled out on first use. Mechanically clean prose. +- Good explicit scoping against its neighbor: "[Bring your own outputs](byo-outputs.qmd) covers the analysis functions in more detail; here the focus is the run itself" tells the reader exactly why this page and that page both touch `icer_table`/`evpi` without repeating each other's content. This is the kind of cross-link the style guide asks for. +- The parameter names (`p_event`, `rr_treat`, `c_event`, `c_treat`, `u_event`) are meaningful short tokens, not single letters, in line with the repository's naming convention for teaching code. + +## Weaknesses and gaps + +1. **The tutorial has never been rendered.** `docs/_notebooks/full-pipeline.ipynb` has zero output cells and no `execution_count` on any code cell (verified by inspecting the notebook JSON directly). The writing-style guide is explicit and mandatory here: "For quarto notebooks or any doc that executes code, render it and verify the prose matches the code output, including plots, after writing" and "Run every executable doc and confirm the prose matches the output before committing." Neither happened. I had to execute the code myself to find out that the numbers the prose gestures at are even the ones you get. + +2. **The central interpretive step is missing: the tutorial never says the ICER exceeds the willingness-to-pay threshold.** After `icer_table(outcomes)` prints an ICER of 121,203.865 for Treatment, the only interpretive sentence is: "Treatment is the more expensive, more effective intervention. Whether its ICER stays below a given willingness-to-pay threshold, and how much resolving the parameter uncertainty would be worth, are the questions the expected value of perfect information (EVPI) and the per-parameter ranking answer next." Two code blocks later, `WTP = 50_000.0` appears with no comparison back to the 121,204 figure already on the page. A reader has to notice, unprompted, that 121,204 > 50,000, meaning Treatment is *not* cost-effective at this threshold and "No treatment" is actually the expected-value-maximizing choice. That single sentence, plainly stated ("Treatment's ICER of about 121,000 exceeds the 50,000 threshold, so no treatment has the higher expected net benefit here"), is exactly the kind of "confirm they reached it" moment the style guide calls for ("state the number or table the analysis produces … and confirm they reached it"), and it is the one place in the page this actually matters. Without it, the reader cannot tell that the low EVPI (47.03) that follows is a direct consequence of the decision not being close, which is the standard reading of EVPI in the field and the single most useful thing this page could teach. + +3. **`evppi_ranking` and the EVPI number get no interpretation at all.** The code block that prints `EVPI at WTP 50,000: 47.03` and the per-parameter ranking (`rr_treat` 9.75, `p_event` 0.70, `c_treat` 0.12, everything else ~0) is followed immediately by the next section heading, "## Checking convergence with running means." The style guide requires "a sentence or two after the output that interprets it: what the number or plot means for the decision, not a restatement of what was computed" for every code block. Here there is no sentence at all, not even a restatement. A reader is left to guess why `rr_treat` dominates the ranking and what an EVPI of 47.03 (per person, though the units are never stated) means in practice. + +4. **The `status` column in `icer_table`'s output is never explained.** Compare to `byo-outputs.qmd`, which handles the identical call ("The `status` column marks dominated (`D`) and extended-dominated (`ED`) interventions; here all three are non-dominated (`ND`)"). `full-pipeline.qmd` prints the same `status` column (both rows read `ND`) but never says what the letters mean. Since this is the reader's first encounter with `icer_table` in the whole tutorial sequence, the explanation belongs here, not two pages later. + +5. **Decision inputs are not lifted to named constants at the top**, contrary to the style guide's explicit instruction: "Lift the decision inputs, the willingness-to-pay threshold, the seed, the iteration count, the time horizon, to named constants near the top of the first code block." Here, the sample size is inlined (`params.sample(4_000, seed=seeds.generator())`), the seed is inlined (`SeedManager(42)`), and `WTP = 50_000.0` is declared midway through the page, in its own code block, rather than up top. `byo-outputs.qmd` does this correctly (`WTP = 30_000.0 # sits between the two frontier ratios, so the decision is uncertain` and `N = 2_000`, both declared at the top of the first code block with a reason given for the WTP choice). `full-pipeline.qmd` should follow that pattern; right now a reader adapting the tutorial has to hunt through three code blocks to find every value they might want to change. + +6. **The opening does not state a target result**, contrary to the guide's mechanic: "Put a specific outcome in the title and on the first screen. The title names what the reader will be able to do, and the opening states the result they are working toward, the target number or figure, before the code that produces it." The title, "Full pipeline," names an artifact/process, not an outcome, and the opening sentence is a list of steps ("specify parameters, sample draws, evaluate a model with `run_psa`, then pass the outcomes to cost-effectiveness and value-of-information analysis") rather than a stated destination. This is precisely the pattern the guide calls out as insufficient: "a list of actions is not the same as a statement of purpose … what they cannot see is why this page exists or what they will be able to do afterward." Contrast the site's own homepage quickstart, which states the payoff up front and follows through with numbers ("All three interventions lie on the frontier here, with Treatment A entering near 11,000 per QALY and Treatment B near 52,000" and "the acceptability curves cross … so EVPI is high there"). + +## Writing-style issues (with quoted lines) + +- Mechanically the page is close to clean: no em-dashes, no exclamation points, headings in sentence case, backticks used consistently for identifiers, no CS jargon, no banned vocabulary. This is a genuine strength worth stating plainly rather than manufacturing nitpicks. +- The one real prose problem is the missing interpretation noted above (items 2 and 3), which is a *what makes a tutorial good* violation more than a sentence-level wording problem: "Show the result the reader is working toward, and confirm they reached it… A reader needs both to see where a section is headed and to have a way to tell that their own run landed in the same place" (writing_style.md, "What makes a tutorial good"). The page shows the numbers but never confirms what they mean. +- Minor: "a Spearman rank correlation can be imposed on any pair of parameters to reflect a known dependence" introduces a mechanism with no example in this tutorial (the correlation argument is not used in the `ParameterSet` call shown). It is demonstrated properly in `parameter-inputs.qmd` and `byo-outputs.qmd`, so this is a forward reference without a payoff on this page; either drop the sentence here or point explicitly to where it is shown, rather than describing unshown functionality. +- Minor: `SeedManager(42)` appears with no explanation of what it does or why 42 versus any other value, on the reader's very first encounter with the class. One clause ("`SeedManager(42)` fixes the random seed so the draws are reproducible") would close the gap cheaply. + +## Concept introduction and sequencing + +- ICER, willingness-to-pay threshold, EVPI, and EVPPI are all introduced here for the first time in the tutorial sequence, and the page does the basics right: it gives the ICER formula in LaTeX, spells out ICER and QALY on first use, and states the general EVPI/EVPPI purpose before the code that computes them. No concept is used before its first definition. That part of the sequencing is sound. +- The larger sequencing concern is overlap with the site's own homepage (`docs/index.qmd`), which a reader reaches before any tutorial page. The homepage "Quickstart" already runs a complete probabilistic sensitivity analysis on an actual model engine (a three-state `MarkovModel`), calls `icer_table`, `plot_ce_plane`, `plot_ceac`, and `evpi` on a threshold grid, and interprets every output in concrete numbers ("Treatment A entering near 11,000 per QALY and Treatment B near 52,000", "the acceptability curves cross … so EVPI is high there"). `full-pipeline.qmd` repeats the same shape of workflow (parameters, sample, `run_psa`, `icer_table`, `evpi`) on a bespoke one-cycle decision tree instead, without acknowledging the overlap or explaining what this page adds beyond what a reader already saw on the homepage. The genuine additions here (`draws.head()`, `outcomes.summary()`, `evppi_ranking`, and the `running_means` convergence check) are real and worth keeping, but the page would be stronger if its opening said so explicitly, e.g., "this tutorial repeats the homepage's workflow but adds the diagnostic steps: inspecting the draw matrix, checking Monte Carlo convergence, and ranking parameters by their value of information," rather than leaving the reader to notice the repetition unprompted. +- Building the "model" as a hand-written decision-tree function rather than one of the four listed engines (the page itself later says "the model engine tutorials cover the model types `run_psa` can run: cohort state-transition, microsimulation, discrete-event, and compartmental") is a defensible pedagogical choice: it demonstrates that `run_psa`'s actual contract is "any function from a draw matrix to `Outcomes`," which is also how a `MarkovModel` instance is consumed on the homepage. This is worth keeping, but a one-clause acknowledgment ("a model engine like the ones covered next is built the same way, but this page keeps the function explicit") would make the choice legible rather than implicit. +- Placement in the sequence (first of the three "getting started" tutorials, before `byo-outputs.qmd` and `parameter-inputs.qmd`) is correct: it is the only page of the three that runs the complete parameters-to-decision workflow, and the other two are explicitly framed as extensions of it ("how to supply results or parameters computed outside the package," per `index.qmd`). No reordering is warranted. + +## Prioritized recommendations + +**Major** + +1. Render `docs/_notebooks/full-pipeline.ipynb` (or the `.qmd` directly) and commit the actual output, per the mandatory writing-style instruction. Right now the notebook has never been executed, so no one has confirmed the printed prose matches the printed numbers. +2. Add the one sentence connecting the ICER to the willingness-to-pay threshold immediately after `icer_table(outcomes)` and `WTP = 50_000.0` appear on the page: state plainly that 121,204 exceeds 50,000, so Treatment is not the cost-effective choice at this threshold, and that this is why the EVPI that follows is small. This is the single highest-value fix: it turns three disconnected numbers into the one decision-relevant insight the whole page is building toward. +3. Add an interpreting sentence after the EVPI/`evppi_ranking` code block: state which parameter dominates the ranking (`rr_treat`) and one clause on what that means for where further data collection would help. +4. Explain the `status` column (`ND`/`D`/`ED`) at its first appearance, here, rather than assuming the reader will meet the explanation later in `byo-outputs.qmd`. + +**Minor** + +5. Lift `WTP`, the sample size (4,000), and the seed (42) to named constants at the top of the first code block, following the pattern already used correctly in `byo-outputs.qmd`. +6. Rewrite the opening to state a target result (a number or a decision) rather than a list of steps, and consider naming the title after an outcome rather than the pipeline itself. +7. Either drop the unshown Spearman-correlation sentence from this page or add a one-line pointer to where it is actually demonstrated (`parameter-inputs.qmd`). +8. Add one clause explaining what `SeedManager(42)` does on its first appearance in the tutorial sequence. +9. Acknowledge, in a sentence, the overlap with the homepage quickstart and state explicitly what this page adds (the draw-matrix inspection, the convergence check, and the EVPPI ranking). diff --git a/devdocs/tutorial-evaluation/eval-markov-vs-microsim.md b/devdocs/tutorial-evaluation/eval-markov-vs-microsim.md new file mode 100644 index 0000000..caea0e4 --- /dev/null +++ b/devdocs/tutorial-evaluation/eval-markov-vs-microsim.md @@ -0,0 +1,54 @@ +# Evaluation: Markov vs microsimulation models (`markov-vs-microsim.qmd`) + +**Slot:** Model engines (3 of 5) + +**Overall verdict:** The comparison is genuinely useful and the numbers are correct (I ran the code myself: 78,614 dollars, 11.740 quality-adjusted life-years (QALYs) for the cohort trace; within half a percent at 40,000 individuals; about an 8% QALY rise and 2% cost rise under frailty), but the page sits awkwardly in the sequence, since it teaches no new engine and its cross-validation section substantially repeats a check `microsim.qmd` already ran. It also breaks two of the project's own explicit rules that neighboring tutorials follow correctly: decision inputs (the frailty variance, population sizes, seed) stay as inline literals rather than named constants, and the frailty variable `z` and the duration attribute `tis` are exactly the uninformative short names CLAUDE.md tells authors to avoid. Rating: **Good**, held back from "Excellent" by the sequencing overlap and the naming/constants slips rather than by any factual error. + +## Strengths + +- **The numbers are correct.** I installed the package and ran the tutorial's code verbatim. The cohort trace gives cost 78,613.52 and QALYs 11.740, matching "78,614 dollars and 11.740 quality-adjusted life-years" once rounded. At 40,000 individuals the homogeneous microsimulation gives cost 78,949.20 (0.43% off) and QALYs 11.769 (0.25% off), both "within half a percent," as claimed. The heterogeneous-frailty run gives a 7.9% QALY rise and a 2.3% cost rise against the cohort, matching "about 8%" and "about 2%." None of this was taken on faith; the printed claims hold. +- **The comparison is pedagogically real, not a contrived example.** Frailty selection, individuals who die early are disproportionately the frail ones, so survivors skew healthier than the cohort's single average-frailty trajectory assumes, is a genuine, well-known phenomenon in survival analysis, and the tutorial demonstrates it with a mean-preserving change (Gamma frailty with mean 1) so the cohort's failure to move is itself informative. Building both engines from one shared `hazards` function and reusing it for the cohort, the homogeneous microsimulation, and the heterogeneous run is a sound design: any divergence in the cross-validation step is attributable to Monte Carlo noise, not to two independently coded models drifting apart. +- **The "neither is more correct" conclusion is earned and stated plainly.** The close: "The cohort model is faster and exact under its assumptions. The microsimulation costs iterations but represents the heterogeneity and history the cohort averages away. Neither is more correct in general; each is valid under its own assumptions." This follows directly from what the page just demonstrated (agreement under the cohort's own assumptions, divergence once those assumptions are violated) rather than asserting a conclusion the reader has to take on faith. +- **Concepts are introduced before use and interpreted after.** Frailty is introduced in prose before the code needs it ("This section assigns each individual a frailty `z`, drawn from a Gamma distribution with mean 1 and variance 0.5") and interpreted by name only after the output ("The reason is frailty selection: ..."). Duration groups get the same treatment: "the microsimulation instead tracks a `duration_groups` counter per individual and reads it directly in the transition function, without restructuring the state space" precedes the code that calls it. +- **Clean on the mechanical rules.** No em-dashes, no exclamation marks, sentence-case gerund headings that are consistently parallel ("Specifying...", "Cross-validating...", "Adding...", "Representing..."), and none of the banned filler words (leverage, comprehensive, robust, streamline, and the rest) appear anywhere on the page. + +## Weaknesses and gaps + +1. **The cross-validation section substantially repeats a check `microsim.qmd` already performed.** That tutorial's "Validating against the cohort closed form" section already builds a hand-computed cohort trace and shows a `MicrosimModel` converging to it "to under 1% at 60,000 individuals," with the explicit rationale "a mismatch here would point to a bug rather than sampling noise." `markov-vs-microsim.qmd`'s "Cross-validating the two engines" section makes the identical methodological point (homogeneous microsimulation converges to a cohort trace as population grows) with a different worked model. A reader who did the tutorials in sequence, as `index.qmd` recommends, sees this argument twice before reaching the genuinely new material (frailty selection, duration groups) two sections later. + +2. **Decision inputs are not lifted to named constants, unlike the linked full script.** The style guide requires: "Lift the decision inputs, the willingness-to-pay threshold, the seed, the iteration count, the time horizon, to named constants near the top of the first code block." Here, the frailty variance (`0.5`), the seed (`seed=1`), and the population sizes (`2_000, 10_000, 40_000`) are all inline literals repeated across three separate code blocks. This is a real regression relative to the page's own reference script: `examples/markov_vs_microsim.py` defines `FRAILTY_VAR = 0.75` and `SEED = 20260705` as named constants exactly where the guide asks for them, but the qmd tutorial that supposedly summarizes that script does not carry the pattern over, and even uses different values (0.5 vs. 0.75 frailty variance; seed 1 vs. 20260705; population sizes 2,000/10,000/40,000 vs. 1,000/5,000/20,000/80,000). A reader who clicks through to "the full script" to adapt it finds different numbers and different function names (`_hazards_from`, `_competing_risks` vs. the tutorial's `hazards`, `rows_from`), not the same code with more scaffolding, which is the pattern `mdm-cohort.qmd`/`examples/mdm_cohort.py` and `microsim.qmd`/`examples/microsim.py` both follow correctly (identical parameter names and values in each pair). + +3. **`z` and `tis` are exactly the uninformative short names CLAUDE.md singles out for avoidance.** The rule reads: "Avoid uninformative single-letter names in teaching code... Two exceptions stay: domain-standard notation, such as the SEIR compartment letters `s, e, i, r, v`, and parameter names that follow a replicated paper's own conventions (`p_S1D`, `r_HS1`, `hr_S1`)." `z` for frailty fits neither exception: this is not a replicated published model (unlike `mdm-cohort.qmd`'s Sick-Sicker parameters, which the exception explicitly protects), and the page never states that `z` is standard shared-frailty notation in survival analysis, so a reader has no way to tell it apart from an arbitrary choice. Worse, `microsim.qmd`, the tutorial this page explicitly builds on, already established the spelled-out name `frailty` as the attribute for the same concept (`pd.DataFrame({"frailty": rng.lognormal(...)})`); this page's silent switch to `z` is also an internal-consistency lapse, not just a naming shortcut. `tis` (`duration_groups={"tis": ("S1", "S2")}`, `attrs["tis"].to_numpy()`) is worse still: it is a made-up abbreviation invented for this page, never spelled out anywhere in the prose, standing for what the code's own docstring convention elsewhere calls `time_in_state`. + +4. **The code inside the qmd has no comments at all**, in contrast to both sibling tutorials and its own linked script. `mdm-cohort.qmd` and `microsim.qmd` both use inline comments to flag non-obvious lines (`# Dead is absorbing`, `# extra Sick -> Dead risk per year already spent sick`), and `examples/markov_vs_microsim.py` is generously commented (`# Healthy: onset (fixed) and background death (frail).`, `# 8% higher death hazard per sick year`). None of that carries into the qmd's code blocks. This matters most in `rows_from`/`hazards`, which pack boolean-mask indexing and broadcasting (`haz[h, 1] = ...; haz[h, 3] = ...` on one semicolon-joined line, `np.errstate(invalid="ignore", divide="ignore")`) that is denser numpy than either prerequisite tutorial uses, with no comment explaining why the `errstate` suppression is needed (an intentional 0/0 that `np.where` overwrites) or what the boolean masks `h, s1, s2` pick out. + +5. **The QALY convergence claim slightly overstates what the printed table shows.** The prose states: "and the gap shrinks further as the population grows." The printed table gives QALY gaps of 0.34% (n=2,000), 0.20% (n=10,000), and 0.25% (n=40,000): the gap narrows from 2,000 to 10,000 but widens slightly from 10,000 to 40,000, a single-seed Monte Carlo wobble rather than a monotonic trend. The statement is true in expectation but not, read literally, of the specific numbers the reader is looking at in the table two lines above it. + +## Writing-style issues (with quoted lines) + +- No punctuation, banned-vocabulary, or heading-parallelism violations were found. +- The code-density issue in item 4 above is a style violation as much as a pedagogical one: the guide's rule "In a code block a reader reads step by step, separate the logical stages... Tightly packed lines are harder to follow than the few extra lines cost" is written about blank lines between stages, but its spirit clearly extends to the semicolon-joined statements inside `hazards`, e.g. `haz[h, 1] = p["r_HS1"]; haz[h, 3] = p["r_HD"] * z[h]`. Neither `mdm-cohort.qmd` nor `microsim.qmd` uses a semicolon anywhere in their code. +- The heading "Cross-validating the two engines" reuses a term ("cross-validation") that has a specific, different meaning in statistics, splitting data to estimate out-of-sample predictive error, which this HEOR-literate readership is more likely to know than a software-engineering audience would. Here the word means something closer to "checking that two independent implementations agree." The page never states this distinction, and the two uses are close enough in subject (checking a model against held-out truth) that a reader with a statistics background could reasonably expect a data-splitting exercise and be confused not to find one. "Verifying the two engines agree" or "Checking the two engines against each other" would avoid the collision. +- Acronym handling is otherwise sound: "quality-adjusted life-years" is spelled out and not abbreviated again on the page, matching the rule to prefer the words when a term appears only once or twice. + +## Concept introduction and sequencing + +- **As the third model-engines tutorial, its prerequisite links are correct.** The opening paragraph names both dependencies explicitly: "It builds on the [cohort state-transition tutorial](mdm-cohort.qmd) and the [microsimulation engine tutorial](microsim.qmd)." `microsim.qmd`'s own closing line points forward to this page with matching language ("Next: Markov vs microsimulation models builds this microsimulation and a cohort twin from the same rates, cross-validates them, and shows what the individual model carries that the cohort averages away."), so the link is bidirectional and consistent. +- **The page teaches no new engine, in tension with how `index.qmd` frames this section.** The section's own description reads: "The cohort state-transition model is the standard engine; the microsimulation, discrete-event, and compartmental engines add the patient history, resource contention, and disease transmission a cohort average cannot represent." That sentence names exactly four engines (cohort, microsimulation, discrete-event, compartmental), matching `mdm-cohort.qmd`, `microsim.qmd`, `des.qmd`, and `seir-vaccination.qmd`; `markov-vs-microsim.qmd` is not mentioned at all, because `MarkovModel` and `MicrosimModel` were both already introduced by the two tutorials it depends on. It is a comparison-and-methods page occupying a slot the surrounding text frames as "one tutorial per model engine." +- **The next tutorial in the sequence does not treat this page as a dependency.** `des.qmd`'s opening paragraph states its own prerequisite explicitly: "the [microsimulation tutorial](microsim.qmd) covers ideas this one builds on: building the model once and running it on draws, and accruing discounted costs and effects over time." It skips `markov-vs-microsim.qmd` entirely, even though it sits directly before `des.qmd` in the listing. The "Next" link at the bottom of this page is one-directional: this page points forward to `des.qmd`, but `des.qmd` does not point back, confirming that the comparison content is not on the load-bearing dependency chain for what follows. That is consistent with treating this page as a well-earned detour or appendix rather than a required link in the engine chain, but the sequence as currently laid out does not say so; a reader following the tutorials strictly in order cannot tell, from the index page or from `des.qmd`, that skipping this one would cost them nothing needed for discrete-event simulation. +- **The notebook has not been rendered**, consistent with the rest of the site (`mdm-cohort.ipynb`, `microsim.ipynb`, and `des.ipynb` are equally unexecuted, all `execution_count: null` with zero outputs), so this is a site-wide gap rather than one specific to this page. It does mean the guide's mandatory "render and verify" step has not happened here either; the numbers check out only because I ran them independently, not because the repository's own build did. + +## Prioritized recommendations + +**Major** + +1. Reconsider the cross-validation section's necessity, or shorten it substantially and point back to `microsim.qmd`'s "Validating against the cohort closed form" instead of re-deriving the same argument. If it stays, add one sentence naming the overlap ("as in the microsimulation tutorial's closed-form check...") so a sequential reader knows why the argument looks familiar. +2. Lift the frailty variance, the seed, and the population-size list to named constants at the top of the first code block (`FRAILTY_VARIANCE = 0.5`, `SEED = 1`, `POPULATION_SIZES = (2_000, 10_000, 40_000)`), matching the pattern `examples/markov_vs_microsim.py` already uses correctly, and reconcile the qmd's parameter values with that script so "the full script" is actually the same worked example. +3. Rename `z` to a spelled-out attribute consistent with `microsim.qmd`'s own `frailty` column, and rename `tis` to something legible (`years_sick` or similar), since neither name meets either of CLAUDE.md's stated exceptions for short names in teaching code. +4. Either add a sentence to `index.qmd`'s "Model engines" description acknowledging this page's different role (a comparison tutorial, not a fifth engine), or move it to a slot whose framing already fits that role. + +**Minor** + +5. Add brief comments to the `hazards`/`rows_from` code, particularly the `np.errstate` suppression and the boolean masks, since this is denser numpy than either prerequisite tutorial introduces and the prose does not walk through the implementation, only the math. +6. Soften "the gap shrinks further as the population grows" to match the printed table, which shows the QALY gap narrow from 2,000 to 10,000 individuals but widen slightly from 10,000 to 40,000 in this single run. +7. Replace "cross-validating" in the section heading and prose with a phrase that does not collide with the statistical meaning of cross-validation, for example "verifying the two engines agree." diff --git a/devdocs/tutorial-evaluation/eval-mdm-cohort.md b/devdocs/tutorial-evaluation/eval-mdm-cohort.md new file mode 100644 index 0000000..2f4adfd --- /dev/null +++ b/devdocs/tutorial-evaluation/eval-mdm-cohort.md @@ -0,0 +1,57 @@ +# Evaluation: Cohort state-transition model (`mdm-cohort.qmd`) + +**Slot:** Model engines (1 of 5); also cross-listed under Replications + +**Overall verdict:** This is a well-motivated page with genuinely verified numbers (I ran the code myself: the deterministic incremental cost-effectiveness ratios (ICERs), the probabilistic sensitivity analysis (PSA) ranking, and the expected value of perfect information (EVPI) all match what the prose claims), clean parallel headings, and no banned vocabulary. It falls short of "Excellent" on craftsmanship rather than substance: the notebook has never been rendered, the transition-matrix code uses the single-letter names `P` and `p` that CLAUDE.md names as the canonical example to avoid, the willingness-to-pay threshold and iteration count are hardcoded rather than lifted to constants (unlike the sibling tutorial that reuses this exact model), and the prose calls the correction method "half-cycle correction" when the code actually specifies Simpson's rule, a distinct option in the package's own API. Rating: **Good**. + +## Strengths + +- **Verified correct.** I installed the package with `uv` and ran the tutorial's code directly. The deterministic run reproduces exactly what the prose states: standard of care costs 151,579.87 for 20.71 quality-adjusted life-years (QALYs), Intervention A is dominated, Intervention B's ICER is 72,987.64 ("about 73,000"), and Intervention AB's is 125,763.79 ("about 126,000"). The package's own test suite (`tests/test_markov.py::test_sick_sicker_matches_published_icers`) pins these same figures against the published paper with tight tolerances, so the "matches exactly" claim is not just internally consistent, it is independently checked. The PSA run preserves the same frontier ranking (standard of care, Intervention B, Intervention AB, with Intervention A dominated), and EVPI at a threshold of 100,000 comes out to 2,757, positive as claimed. +- **The replication motivates itself rather than sitting beside the teaching as an afterthought.** "Reproducing a known answer first, before trusting the model on a new question, is the standard check for any cost-effectiveness model" gives the reader a reason to care about the validation step, not just a statement that one exists. +- **The confirmation sentence is the style guide's own example, verbatim.** `writing_style.md` illustrates "say plainly whether it matched" with the exact phrase "the result should match the published table exactly. It does," and illustrates stating a target result with "the frontier runs standard of care, then Intervention B at about 73,000 per QALY." Both appear almost word for word in this tutorial's Section 2. Whether this page was the source of those examples or just the best-matching instance, it is a faithful demonstration of the rule. +- **The math is introduced at the right depth and is checkable against the code.** The prose states the rate-to-probability conversion $p = 1 - e^{-r\,\Delta t}$, ties `hr_S1` and `hr_S2` to multiplying the death rate before conversion, and gives the cohort-trace update $m_{t+1} = m_t P$. I checked both claims against the code: `p_S1D = r2p(p["r_HD"] * p["hr_S1"])` does multiply before converting, and the transition matrix rows are built exactly as the update equation implies. +- **No redundancy with the getting-started tutorials that precede it, despite reusing their objects.** `ParameterSet`, `.sample`, `run_psa`, and `icer_table` are taught in `full-pipeline.qmd`; the cost-effectiveness acceptability curve and the `ND`/`D`/`ED` status column are taught in `byo-outputs.qmd`; EVPI is introduced in `full-pipeline.qmd` too. This page correctly does not re-teach any of that, and the phrase "From there the analysis is the same as for any other model" signals the reuse without repeating the explanation. The one new concept, building `MarkovModel`/`CohortSpec` from a transition matrix and reward vectors, gets the bulk of the prose. That is the right division of attention for the first engine tutorial. +- **Sound acronym handling.** "Incremental cost-effectiveness ratio" is used twice, in full, and never abbreviated to "ICER" since the acronym is not needed again on this page, matching the guide's rule exactly ("when a term appears once or twice, use the words instead of the acronym"). QALY and EVPI are each spelled out on first use. +- **Clean on the mechanical rules.** No em-dashes, no exclamation marks, headings are consistently gerund-form and parallel ("Specifying the four-state model," "Reproducing the published base case," "Running the probabilistic sensitivity analysis"), and none of the banned filler words (leverage, comprehensive, robust, streamline, and so on) appear. + +## Weaknesses and gaps + +1. **The notebook has never been rendered.** `docs/_notebooks/mdm-cohort.ipynb` has zero output cells and no `execution_count` on any code cell (confirmed by inspecting the notebook JSON directly). The style guide is explicit and non-optional here: "For quarto notebooks or any doc that executes code, render it and verify the prose matches the code output, including plots, after writing," and "Run every executable doc and confirm the prose matches the output before committing." Neither happened. The fact that the numbers happen to be correct (verified above by running the code myself) is luck, not process; nothing in the repository currently proves the page's own render matches its prose. + +2. **`P` and `p` are exactly the single-letter names CLAUDE.md singles out as the thing to avoid.** The project's own naming rule reads: "Avoid uninformative single-letter names in teaching code... use a meaningful short word or `snake_case` name instead... `P` for a transition matrix becomes `transition_matrix`." The tutorial's `model` function does precisely this: `P = np.zeros((4, 4))` for the transition matrix, and the function signature `def model(p, intervention):` uses `p` for the parameter draw row throughout. Compare `examples/mdm_cohort.py`, the full script this page links to and claims to summarize: it uses `params: pd.Series` (spelled out) and never introduces a bare `P`. The exception in CLAUDE.md protects paper-specific *parameter names* (`p_S1D`, `r_HS1`, `hr_S1`), which this page correctly keeps, but `P` and `p` are local implementation variables invented for this code block, not the source article's notation, so the exception does not cover them. + +3. **Decision inputs are not lifted to named constants**, contrary to the guide's explicit instruction: "Lift the decision inputs, the willingness-to-pay threshold, the seed, the iteration count, the time horizon, to named constants near the top of the first code block." Here, `n_cycles=75` is inlined in the `MarkovModel` call, the iteration count is inlined (`params.sample(1000, ...)`), the seed is inlined (`SeedManager(20260705)`), and the willingness-to-pay threshold appears only as a literal inside the `evpi` call (`evpi(outcomes, 100_000.0)`). This is a real inconsistency within the tutorial set: `full-pipeline.qmd` lifts `WTP = 50_000.0`, `byo-outputs.qmd` lifts `WTP = 30_000.0` and `N = 2_000` with a comment explaining the choice, and `voi.qmd`, which reuses this exact Sick-Sicker model and even copies most of this page's code verbatim, lifts `WTP = 100_000.0` at the top of its first code block. `mdm-cohort.qmd` is the outlier among its own neighbors on a rule the guide states as a requirement, not a suggestion. + +4. **The prose names the wrong correction method.** Section 1 states: "discounted and summed over the horizon with a half-cycle correction." The code, however, passes `cycle_correction="simpson"` to `MarkovModel`. The package's own docstring for `gen_wcc` (`src/heormodel/models/markov.py`) distinguishes these as separate, mutually exclusive options: `"simpson"` for "Simpson's 1/3 rule," `"half_cycle"` for "half weights on the first and last point," and gives different numeric weights for each (`[0.5, 1.0, 1.0, 1.0, 0.5]` for half-cycle vs. `[0.333, 0.667, 1.333, 0.667, 0.333]` for Simpson's, at 4 cycles). These are not interchangeable names for the same idea; Simpson's rule is a different, more accurate quadrature. A reader who reads the code after the prose will find a mismatch between the term used and the option actually invoked. + +5. **The willingness-to-pay threshold is never connected to the stated ICERs before EVPI is computed.** Section 2 states the frontier ICERs (about 73,000 for Intervention B, about 126,000 for Intervention AB) without mentioning any threshold. The number 100,000 first appears three paragraphs later, inside the `evpi` call, with no sentence stating that 100,000 falls between the two ICERs and that Intervention B is therefore the cost-effective choice in the deterministic base case. The explanation that follows the EVPI print ("Intervention AB... is the best choice in some draws but not others") is correct and useful, but it assumes the reader has already noticed, unprompted, where 100,000 sits relative to 73,000 and 126,000. One sentence stating the deterministic decision at that threshold, before computing EVPI, would make the "why EVPI is positive" explanation land rather than requiring the reader to do the arithmetic themselves. + +6. **The opening paragraph does not link back to the getting-started tutorials it silently depends on.** The page uses `ParameterSet`, `.sample`, `run_psa`, `icer_table`, and `evpi` without reintroducing any of them, which is the right call given they were taught in `full-pipeline.qmd` and `byo-outputs.qmd`. But the guide's rule for a page's first paragraph is: "what it covers and where to go next... either the page is self-contained or it links to the page that supplies the missing piece." This page's opening links forward to the replication gallery and to the full script, but nothing points a reader who arrived here first (skipping the recommended order) back to where `ParameterSet` or `run_psa` were introduced. `microsim.qmd`, the next engine tutorial, does this correctly in the other direction ("It runs the same `run_psa`... as the Markov cohort model tutorial, so the two are worth reading as a pair"). + +## Writing-style issues (with quoted lines) + +- No violations of the punctuation, vocabulary, or heading rules were found: no em-dashes, no exclamation marks, sentence-case headings, backticks used consistently for identifiers, and none of the banned words (leverage, delve, comprehensive, robust, streamline, and the rest). +- The precision slip in item 4 above is a writing-style problem as much as a correctness one: "with a half-cycle correction" is confident, unhedged prose about a mechanism the code does not use. The guide's rule that a "why" sentence "earns its place when it follows directly from the code" cuts both ways; a factual description of what the code does needs the same discipline. +- The code itself reads more like compressed reference code than teaching code, in tension with the audience this guide defines: "a reader ... who builds cost-effectiveness models and writes code to do it, but who is not a software engineer." Lines such as `P[0, 0], P[0, 1], P[0, 3] = (1 - p_HD) * (1 - p_HS1), (1 - p_HD) * p_HS1, p_HD` pack a triple assignment and three nested products onto one line. `examples/mdm_cohort.py`, the linked full script, expresses the identical logic one assignment per line (`P[0, 0] = (1 - p_HD) * (1 - p_HS1)`, then `P[0, 1] = ...`, then `P[0, 3] = ...`) and is markedly easier to trace cell by cell against the transition-matrix diagram a HEOR reader would sketch on paper. The tutorial's version appears optimized for a shorter code block rather than for a reader stepping through it for the first time. + +## Concept introduction and sequencing + +- All the shared analysis-layer objects the task of evaluating this page might expect to see introduced here, `ParameterSet`, sampling, `run_psa`, `icer_table`, the cost-effectiveness acceptability curve, EVPI, were in fact already introduced in the three getting-started tutorials that precede it (`full-pipeline.qmd` for the first four and EVPI, `byo-outputs.qmd` for the acceptability curve and the dominance/`status` column). That means this page's actual job is narrower and better-defined than "teach the shared workflow plus the engine": it only needs to teach the engine, `MarkovModel` and `CohortSpec`, and it spends essentially all of its prose doing exactly that. This is good sequencing, not a gap; a reader who followed the intended order arrives here already fluent in everything except the cohort mechanics. +- The general contract of `transitions_and_rewards` is stated once, thinly but adequately: "The `transitions_and_rewards` function below returns each intervention's transition matrix and per-state payoffs from a single parameter row." A reader building their own model has to infer from the code, rather than from a stated rule, that `CohortSpec`'s three positional arguments are transition matrix, cost vector, effect vector, in that order. One clause naming that order explicitly would remove the inference step. +- The dual role, entry engine tutorial and published replication, works better than a first read suggests, because the analysis-layer scaffolding is handled elsewhere (previous point) and because the guide itself rules out the alternative: "teach against a real model... not on a synthetic construct built so that an analytic answer exists to check against." A hypothetical simplified, non-replication cohort example would violate that rule for no real gain. The genuine cost of the dual role is that a reader meeting the engine for the first time must also decode the source paper's dense notation (`r_HS1`, `hr_S1S2_trtB`, `hr_S1`) simultaneously with the engine's API; CLAUDE.md explicitly protects those names as the paper's own convention, so this is an inherent, accepted tension of the design rather than a fixable mistake. +- No redundancy was found with `full-pipeline.qmd`: that page builds a bespoke decision-tree function, never a cohort model, so the two tutorials do not duplicate the same worked example, only the workflow shape around it (which the guide explicitly endorses repeating). + +## Prioritized recommendations + +**Major** + +1. Render `docs/_notebooks/mdm-cohort.ipynb` and confirm the printed output matches the prose, per the mandatory writing-style step. This is currently unverified in the repository even though I independently confirmed the numbers are correct by running the code myself. +2. Replace `P` and `p` in the `model` function with meaningful names (`transition_matrix` and `params`, matching `examples/mdm_cohort.py`), consistent with CLAUDE.md's own worked example of this exact rule. +3. Lift `n_cycles`, the PSA iteration count, the seed, and the willingness-to-pay threshold (100,000) to named constants near the top of the first code block, matching the pattern already used correctly in `full-pipeline.qmd`, `byo-outputs.qmd`, and `voi.qmd` (which reuses this same model). +4. Correct "half-cycle correction" to name Simpson's rule, or state that `cycle_correction="simpson"` is a more accurate variant of the within-cycle correction family that includes half-cycle correction as a separate option; do not use the two terms interchangeably. + +**Minor** + +5. Add one sentence stating where the 100,000 threshold sits relative to the two frontier ICERs (73,000 and 126,000) before computing EVPI, so the "why EVPI is positive" explanation that follows does not require the reader to do that comparison unprompted. +6. Add a clause naming `CohortSpec`'s argument order (transition matrix, cost vector, effect vector) explicitly, rather than leaving it to be inferred from the `return` statement. +7. Add a one-clause back-link in the opening paragraph to `full-pipeline.qmd` or `byo-outputs.qmd` for a reader who has not yet met `ParameterSet`, `run_psa`, or `icer_table`, mirroring how `microsim.qmd` links back to this page. diff --git a/devdocs/tutorial-evaluation/eval-microsim.md b/devdocs/tutorial-evaluation/eval-microsim.md new file mode 100644 index 0000000..e98acf7 --- /dev/null +++ b/devdocs/tutorial-evaluation/eval-microsim.md @@ -0,0 +1,52 @@ +# Evaluation: Microsimulation engine (`microsim.qmd`) + +**Slot:** Model engines (2 of 5) + +**Overall verdict:** The engine-specific teaching is sound: the opening states what the reader will be able to build and gives two concrete, checkable reasons to reach for a microsimulation (a `frailty` attribute and a duration-dependent hazard), and every number I re-ran (the incremental cost-effectiveness ratio, the expected value of perfect information (EVPI), the per-parameter ranking, the closed-form agreement) matched the prose exactly. It is let down by one structural mistake the style guide names directly: the "Validating against the cohort closed form" section reproduces `tests/test_microsim.py::test_converges_to_cohort_closed_form` almost line for line, the exact "ground-truth scaffolding" the guide says belongs in the test suite, not the tutorial, and it pre-empts a comparison the very next tutorial (`markov-vs-microsim.qmd`) makes far more thoroughly. Combined with inlined decision constants that regress behind the linked example script and single-letter names in that same section, this is **Good**, not **Excellent**. + +## Strengths + +- **The opening earns the "when to reach for this" test.** "the model type to use when outcomes depend on a patient's history or on heterogeneity between patients, neither of which a cohort average preserves" names the two real reasons and the model that follows uses both: `frailty` (heterogeneity) and `time_in_state` feeding a rising Sick-to-Dead hazard (history). A reader deciding whether their own problem needs this engine has an actual test to apply, not an adjective. +- **Every claim I checked against a live run held.** I installed the package with `uv` and ran `examples/microsim.py`: the incremental cost-effectiveness ratio comes out to 10,735, consistent with the tutorial's claim that the willingness-to-pay threshold of 11,000 sits "near the base-case" ratio so that "the decision is genuinely uncertain." EVPI at that threshold is 2,390.5, positive as claimed. The expected value of partial perfect information (EVPPI) ranking is led by `c_treat` (1,510.4) then `rr_tx` (1,238.9), matching "the treatment cost and its effect `rr_tx` lead the ranking" exactly. I also reran the closed-form comparison block directly: the microsimulation is within 0.39% (cost) and 0.52% (quality-adjusted life-years, QALYs) of the cohort reference at 60,000 individuals, consistent with "The two agree to under 1%." +- **The reproducibility claim is not a plausible-sounding guess; it is the package's documented contract.** "each iteration is seeded by its position in the draw matrix, so the two agree exactly" matches `run_psa`'s own docstring verbatim in substance: "Because each iteration draws a stream keyed by its index, the numbers... are identical whether the run is parallel or sequential." This is exactly the kind of verified "why" sentence the guide asks for. +- **The branching-versus-decision-levers section is a genuine, non-obvious modeling decision, explained with a rule a reader can apply elsewhere.** "Branch on the name when the arms differ in structure or in which model function runs... Use an `Intervention` with decision levers for a numeric scenario knob the model already reads as a parameter" gives a real criterion, and the warning against a fake parameter flag ("Do not encode an arm as a fake parameter... such a flag is not part of the `ParameterSet`, so `evppi_ranking` and the deterministic sensitivity analyses never see it") is a concrete, checkable consequence rather than a style preference. +- **Headings are gerund-form and parallel throughout**: "Specifying individual-level transition probabilities and utilities," "Configuring once, evaluating on draws," "Branching on the intervention name versus using decision levers," "Analyzing cost-effectiveness and value of information," "Switching to a continuous-time clock." No em-dashes, no exclamation marks, no banned filler vocabulary (leverage, comprehensive, robust, streamline, and so on) anywhere on the page. +- **The continuous-time closing paragraph correctly defers rather than duplicates.** Three sentences describe `MicrosimModel.continuous`'s mechanism (sampled event times, earliest taken, continuous accrual, per-year reward rates) without a redundant worked example, in line with the guide's instruction to "defer side complications to the end rather than interleaving them." + +## Weaknesses and gaps + +1. **"Validating against the cohort closed form" is the ground-truth scaffolding the guide explicitly rules out, and it is not a paraphrase, it is close to a copy of an existing test.** The guide states: "Confirming that an estimator matches its closed form is the job of the test suite; carrying that scaffolding into a tutorial roughly doubles its length and teaches machinery the reader will never write." `tests/test_microsim.py::test_converges_to_cohort_closed_form` already performs this exact check, with the same three-state transition matrix (`[[0.80, 0.15, 0.05], [0.0, 0.90, 0.10], [0.0, 0.0, 1.0]]` in the tutorial versus `[[0.80, 0.15, 0.05], [0.00, 0.90, 0.10], [0.00, 0.00, 1.00]]` in the test), the same cost and effect vectors (`[1_000.0, 3_000.0, 0.0]` and `[1.0, 0.6, 0.0]` in both), the same population of 60,000, and the same 40-cycle horizon. The tutorial did not adapt this scaffolding for teaching, it inlined the test. +2. **That section also duplicates work the next tutorial in sequence does better.** `markov-vs-microsim.qmd` cross-validates a `MarkovModel` and a `MicrosimModel` built from the same rates, at three population sizes, then extends the comparison to heterogeneous frailty and duration-dependent hazards, the exact two features this tutorial's own model uses. A reader who continues to the next page immediately re-reads a more careful version of the check they just saw. The guide's instruction that "Quarto pages link to each other rather than repeating content" argues for cutting this section here and letting `markov-vs-microsim.qmd` carry the validation, with at most a forward link ("the next tutorial cross-validates this engine against a cohort twin"). +3. **Decision inputs are inlined rather than lifted to named constants, regressing behind the linked example script.** The guide requires: "Lift the decision inputs... the iteration count, the time horizon, to named constants near the top of the first code block." `examples/microsim.py`, the script this page claims to walk through, does exactly that: `N = 256 # PSA iterations`, `POP = 800 # individuals per iteration`, `HORIZON = 30 # cycles (years)`. The tutorial's own code block drops this discipline: `draws = parameters.sample(256, seed=seeds.generator())` and `n_individuals=800, ..., n_cycles=30` appear as bare literals. `WTP = 11_000.0` is correctly lifted later in the page, which makes the earlier omission look like an oversight rather than a deliberate choice. +4. **The validating section's code uses exactly the single-letter names CLAUDE.md names as the canonical violation.** `P = np.array(...)` for the transition matrix is the literal example CLAUDE.md gives of what to avoid ("`P` for a transition matrix becomes `transition_matrix`"), and the loop `p, total_c, total_e = ...; for c in range(horizon + 1): w = 0.5 if c in (0, horizon) else 1.0` reuses `p` for the occupancy vector, `c` for the cycle index, and `w` for the half-cycle weight, none of which is domain-standard notation or a replicated paper's own parameter name, the two exceptions CLAUDE.md allows. +5. **The notebook has never been rendered.** `docs/_notebooks/microsim.ipynb` has no `execution_count` and no output on any code cell (confirmed by inspecting the notebook JSON directly); every code cell, including the one producing the closed-form comparison table, is unexecuted. The guide is explicit and non-optional: "For quarto notebooks or any doc that executes code, render it and verify the prose matches the code output... after writing" and "Run every executable doc and confirm the prose matches the output before committing." I confirmed the numbers are in fact correct by running the code myself, but that is independent verification standing in for a process step the repository itself has not completed. +6. **`evppi_ranking` is used before the concept it computes is taught anywhere, and without a forward link to where it will be.** This page is the first tutorial in the whole sequence to call `evppi_ranking` (`mdm-cohort.qmd`, the tutorial immediately before it, computes only `evpi`). The prose gives a one-line functional gloss ("the per-parameter ranking below is informative"; after the output, "they determine whether treatment's incremental cost-effectiveness ratio stays below the threshold") but never states what the ranking measures, the value of resolving one parameter's uncertainty while the rest remain uncertain, which `voi.qmd` defines formally with its own formula, several tutorials later, on a different model. `mdm-cohort.qmd` handles the analogous situation correctly when it introduces EVPI: it links forward explicitly ("The [value of information](voi.qmd) tutorial takes this same model further"). This page has no equivalent link for EVPPI. + +## Writing-style issues (with quoted lines) + +- No em-dash, exclamation mark, or banned-vocabulary violations were found anywhere on the page. +- "common random numbers (the engine default)" is the first use of that term anywhere in the tutorial sequence, and it gets a single parenthetical rather than the sentence of explanation a first use usually receives elsewhere on this page (compare the fuller treatment given to `time_in_state`: "The engine maintains `time_in_state` and passes it, alongside the sampled attributes, to the `transition_probabilities` and `state_rewards` functions"). The clause that follows ("so the incremental result reflects the treatment effect rather than sampling noise") does convey why it matters, so this is a minor thinness rather than an omission. +- The single-letter names flagged in weakness 4 (`P`, `p`, `c`, `w`) are also a style violation independent of whether the section should exist: CLAUDE.md's own worked example of the rule it wants followed is `P` for a transition matrix, and this page's validating section uses precisely that name. +- Section headings are consistently gerund-form and parallel ("Specifying...", "Configuring...", "Branching...", "Analyzing...", "Validating...", "Switching..."), matching the guide's requirement that parallel sections carry parallel headings. +- Acronym handling is correct: "expected value of perfect information" and "expected value of partial perfect information" are never abbreviated in prose (the acronyms EVPI/EVPPI appear only inside an f-string the code prints, which is output, not prose), consistent with "when a term appears once or twice, use the words instead of the acronym." + +## Concept introduction and sequencing + +- The hand-off from `mdm-cohort.qmd` is clean where it matters most: the opening sentence states the page "runs the same `run_psa`, cost-effectiveness, and value-of-information calls as the [Markov cohort model tutorial]," so `ParameterSet`, sampling, and `icer_table` are correctly not re-taught. The "Analyzing cost-effectiveness and value of information" section reinforces this directly: "From here nothing is engine-specific: the same `icer_table`, `evpi`, and `evppi_ranking` calls used for a cohort model apply to this simulation's `Outcomes`." This is the shared-workflow repetition the guide calls a feature, not a flaw, done correctly. +- The one place the hand-off is not clean is EVPPI, covered as weakness 6 above: this page is where the reader meets the per-parameter ranking for the first time, and the formal definition is three tutorials away (`des.qmd`, `seir-vaccination.qmd`, `dsa.qmd` all come first) attached to a different model. A reader who reads tutorials in order sees a working tool before its definition, with no signpost that the definition is coming. +- `attrs`, `time_in_state`, and the individual-versus-cohort attribute distinction are introduced before the code that uses them, in the paragraph preceding the first code block, which is the right order. +- The validating section (weaknesses 1-2) is also a sequencing problem, not just a scaffolding problem: it does the cohort-versus-microsimulation comparison shallowly (one comparison, no heterogeneity, no history) immediately before `markov-vs-microsim.qmd` does the same comparison thoroughly (three population sizes, then heterogeneous frailty, then duration groups) using the same two features this tutorial's own model introduces. Removing it here would tighten this page and give the next one a cleaner setup rather than a repeated one. + +## Prioritized recommendations + +**Major** + +1. Delete "Validating against the cohort closed form." It duplicates an existing test almost verbatim and pre-empts the more thorough comparison `markov-vs-microsim.qmd` performs next. Replace it with at most one sentence noting that the next tutorial cross-validates this engine against a cohort model built from the same rates. +2. Lift the PSA iteration count (256), the individual count (800), and the cycle horizon (30) to named constants near the top of the first code block, matching `examples/microsim.py`'s own `N`, `POP`, and `HORIZON`. +3. Add one sentence, where `evppi_ranking` is first called, stating what it measures (the value of resolving one parameter's uncertainty while the rest remain uncertain) and a forward link to `voi.qmd`, mirroring how `mdm-cohort.qmd` forward-links when it introduces EVPI. +4. Render `docs/_notebooks/microsim.ipynb` and confirm the printed output, including the closed-form comparison table, matches the prose, per the mandatory render-and-check step. + +**Minor** + +5. If the validating section is kept in any form, rename `P`, `p`, `c`, and `w` to `transition_matrix`, `occupancy`, `cycle`, and `weight`, consistent with CLAUDE.md's own worked example of this exact rule. +6. Expand the first mention of common random numbers by one clause describing the mechanism (the two arms evaluate the same simulated individuals under the same random draws), matching the fuller first-use treatment given to `time_in_state` earlier on the same page. diff --git a/devdocs/tutorial-evaluation/eval-parameter-inputs.md b/devdocs/tutorial-evaluation/eval-parameter-inputs.md new file mode 100644 index 0000000..438e94d --- /dev/null +++ b/devdocs/tutorial-evaluation/eval-parameter-inputs.md @@ -0,0 +1,44 @@ +# Evaluation: Parameter inputs from data (`parameter-inputs.qmd`) + +**Slot:** Getting started (3 of 3) + +**Overall verdict:** A well-written page at the sentence level, careful about verifiable claims and free of the style guide's banned habits, but it has one unused import that undercuts its own opening promise, no lifted constants, a stray file write with no cleanup, and it sits outside the site's actual "Next" reading path: the preceding tutorial's forward link skips it, and the one page that points into it does so backward, from the calibration section. Rating: Good. + +## Strengths + +- The opening states the goal in the guide's own preferred form: "This tutorial shows how to turn each of those three sources into a parameter draw matrix with `single_draw` (via `ParameterSet.at_means`), `read_draws`, and `resample_posterior`." This names what the reader will be able to do, not a list of installs and runs. +- Every section opens with a full sentence of purpose and closes with a sentence interpreting the output, matching the guide's three-part section shape. For example, after the CSV section: "The summary is the same probabilistic analysis a native `ParameterSet` sample would give; `read_draws` changes only where the draws come from, not how they are analyzed." +- Claims are verifiable and checked out against the source. "It is `single_draw(params.means().to_dict())` underneath" matches `ParameterSet.at_means`'s own docstring ("Equivalent to `single_draw(self.means().to_dict())`") exactly. Running `examples/parameter_inputs.py` confirms the direction of every interpretive claim: the resampled mean of `p_response` (0.360) exceeds the grid mean (0.348), and the new drug's expected QALYs rise from 8.42 to 8.44 under the reweighted posterior. +- The synthetic weight column is honestly flagged as illustrative: `grid["weight"] = np.exp(4.0 * grid["p_response"]) # favor higher response`, satisfying the guide's rule to make illustrative values unmistakable. +- No banned vocabulary, no em-dashes or exclamation marks, sentence-case headings, and all four section headings are grammatically parallel gerunds ("Specifying", "Running", "Reading", "Resampling"). +- The closing paragraph foreshadows calibration correctly and without repetition: "The calibration workflow produces such a posterior with `abc_calibrate` and mixes it with literature draws through `mix_draws`," pointing forward rather than re-explaining ABC here. + +## Weaknesses and gaps + +- **`single_draw` is imported but never called.** The import line pulls in `single_draw`, and the prose tells the reader to "call `single_draw` directly when the point values come from somewhere other than a `ParameterSet`, a published base case, for instance," but no code block ever does so; the only call shown is `params.at_means()`. A reader who came to this page specifically for the "published base case" use case the prose describes never sees it demonstrated. (The companion script, `examples/parameter_inputs.py`, does call `single_draw({...})` on line 75, but discards the result without printing it, so even there the use case is not actually shown.) +- **No named constants.** The style guide asks that decision inputs, seeds, and sample sizes be lifted to named constants near the top of the first code block, as `byo-outputs.qmd` does with `WTP = 30_000.0` and `N = 2_000`. Here, `seed=1`, `seed=2`, `seed=7`, `n=2_000`, `n=500`, and the reweighting exponent `4.0` are all inline. A reader adapting the CSV or resampling section to their own analysis has to hunt inside the code for the values to change. +- **A file is written to the working directory with no cleanup.** `external.to_csv("external_draws.csv")` writes a real file relative to wherever the page executes (a Colab runtime or a reader's own directory), unlike the example script, which writes into a dedicated `output/` folder it creates. This is the only tutorial in the `docs/tutorials/` directory that writes to disk at all, and it leaves litter behind. +- **The variable name `grid` is misleading.** `grid = params.sample(500, seed=2)` is a random Monte Carlo sample, not a grid of points; the name suggests a deterministic quadrature grid, which is the opposite of what is being reweighted here. CLAUDE.md's naming guidance for teaching code (meaningful names over decode-it-yourself abbreviations) argues for something like `candidate_draws` or `unweighted_draws`. +- **The "raises the new drug's expected QALYs" claim is not paired within its own section.** It is true (verified above), but the reader can only confirm it by mentally comparing this section's summary table against the base-case and CSV sections several screens earlier, which used different sample sizes and seeds. The style guide's "number already printed just above" allowance is a stretch here since the comparison spans non-adjacent output blocks. + +## Writing-style issues (with quoted lines) + +None of the banned patterns (em-dashes, exclamation marks, casual idiom, anthropomorphizing, the vocabulary blocklist) appear on this page; a full read against the guide's checklist turned up no sentence that needed rewriting for voice. The only style-adjacent issue is structural rather than sentence-level: the page's opening sentence packs three function names into one clause, "with `single_draw` (via `ParameterSet.at_means`), `read_draws`, and `resample_posterior`," which is dense but accurate, and the guide's caution against "an inventory of function calls" as an opening is arguably brushed rather than violated, since the sentence's main verb is the purpose ("turn...into a parameter draw matrix"), not a list of installs and runs. + +## Concept introduction and sequencing + +The three methods are introduced in increasing order of statistical weight: a point-value base case, a format round-trip through CSV, then a weighted resampling that actually changes the answer. That progression is pedagogically sound and each method is self-contained, so the "teach one method per tutorial" rule is bent rather than broken: the throughline (three ways to get an external source into the one `run_psa`-ready draw matrix) is real, and it matches this section's own stated scope in `docs/tutorials/index.qmd`: "how to supply results or parameters computed outside the package." Placing `resample_posterior` in "Getting started," well before the calibration section, is defensible on that basis, and the writing style guide's own reader-assumptions section states the target reader already knows what a prior and a posterior are, so the term is not undefined jargon for this audience. + +The real sequencing problem is not conceptual but navigational, and it is a genuine defect worth fixing regardless of which page "owns" it: + +- `byo-outputs.qmd`'s own "Next" pointer reads "Next: the [Markov cohort model](mdm-cohort.qmd) builds the standard state-transition model...", and it skips `parameter-inputs.qmd` entirely, even though `docs/tutorials/index.qmd` lists parameter-inputs.qmd as the third "Getting started" page, directly after byo-outputs.qmd. A reader following "Next" links rather than the tutorials index grid never organically reaches this page. +- `calibration-workflow.qmd`'s "Next" pointer reads "Next: [parameter inputs from data](parameter-inputs.qmd) covers the base-case, imported, and posterior draw matrices that feed the same analysis," which sends a reader who just finished the first calibration tutorial backward into a "Getting started" page instead of forward to `calibrate-abc.qmd`, the next tutorial in its own group per the index. +- Net effect: `parameter-inputs.qmd` is an orphan in the site's actual click-through path. Nothing forward-correct points into it; the one inbound "Next" link that does exist points the wrong direction from the wrong section. + +## Prioritized recommendations (major first, then minor) + +1. Fix the two broken "Next" pointers: change `byo-outputs.qmd`'s "Next" to `parameter-inputs.qmd`, and change `calibration-workflow.qmd`'s "Next" to `calibrate-abc.qmd`. This is the highest-value fix since it is the difference between the tutorial being reachable at all through normal navigation. +2. Either call `single_draw` directly in a code block (for example, on a hand-specified dict, as the prose already promises) or drop it from the import and rewrite the sentence to describe `at_means` without promising a demonstration that never appears. +3. Lift `seed=1`, `seed=2`, `seed=7`, the two sample sizes, and the `4.0` reweighting exponent to named constants at the top of the first code block, matching the pattern already used in `byo-outputs.qmd` and `full-pipeline.qmd`. +4. Rename `grid` to something that does not imply a deterministic grid, such as `candidate_draws`, and write the CSV into a throwaway output directory (or clean it up after reading it back) instead of the working directory. +5. Minor: consider showing the base-case-only or grid-only QALY figure adjacent to the resampled one in the last section, so the "raises the new drug's expected QALYs" claim is confirmed by numbers in the same section rather than by recalling an earlier one. diff --git a/devdocs/tutorial-evaluation/eval-seir-vaccination.md b/devdocs/tutorial-evaluation/eval-seir-vaccination.md new file mode 100644 index 0000000..86f40fb --- /dev/null +++ b/devdocs/tutorial-evaluation/eval-seir-vaccination.md @@ -0,0 +1,60 @@ +# Evaluation: Compartmental transmission model (`seir-vaccination.qmd`) + +**Slot:** Model engines (5 of 5) + +**Overall verdict:** The physics of the model is taught well: the force of infection, the R0-to-beta relationship, and the two reward channels are each introduced with a full sentence before the code that uses them, and every number I reran matched the prose. But the page was adapted from `examples/seir_vaccination.py` in a way that lost ground the script already had (named `HORIZON` and `WTP` constants became bare literals, and the willingness-to-pay threshold used for the cost-effectiveness acceptability curve, 30,000, no longer matches the 50,000 used two code blocks later for the expected value of perfect information (EVPI)), and the page is a navigational dead end: no "Next" link, no back-link from any other tutorial, and the last engine tutorial's own `des.qmd` "Next" link skips over it entirely. The companion notebook was never executed and its prose has not been kept in sync with the page, down to two literal typos. Rating: **Needs work**. + +## Strengths + +- **The force of infection is defined before it is used.** "The coupling between people enters through the force of infection, the rate at which a susceptible person becomes infected. It is proportional to the current infectious prevalence, $\lambda(t) = \beta\, I / N$" precedes the code block that computes `foi = beta * i / (...)`, exactly the sequencing the style guide asks for. +- **The R0-to-beta relationship is stated and used correctly.** "The transmission rate follows from the basic reproduction number, $\beta = R_0\,\gamma$, which fixes how many secondary infections one case produces in a fully susceptible population" matches the code (`beta = p["R0"] * p["gamma"]`), and I confirmed the claim this supports: with `SeedManager(20260711)` and 1,000 draws in the probabilistic sensitivity analysis, `R0`'s expected value of partial perfect information (EVPPI, about 2,358,000) dwarfs every other parameter's (0 for `u_I`, `c_case`, `gamma`, `sigma`), consistent with "the parameter the epidemic is most sensitive to." +- **The two reward channels are explained, not just coded.** "Quality-adjusted life-years accrue on compartment occupancy through `state_effect`... Costs fall on flows rather than states: `event_rates` returns the per-year rate of two events" gives the reader the state-occupancy-versus-event-flow distinction before the `ODESpec` call that instantiates it, which is the one piece of `ODEModel`'s design a reader cannot get from the code alone. +- **The trajectory-inspection step is motivated, not just run.** "`trajectory` integrates the compartments for one parameter set and returns them over time, which is useful for inspecting the model before relying on its cost-effectiveness output" states why this step exists (a sanity check before trusting the downstream numbers), and the interpretation after the plot is both correct and specific. +- **Every number I reran matched the prose.** Running the tutorial's exact `seir` function with `base = pd.Series(dict(R0=1.8, sigma=4.0, gamma=6.0, nu=1.0, c_vacc=200.0, c_case=150.0, u_I=0.7))` gives a final recovered count of 73,185 without vaccination and 70 with it, matching "from about 73,000 to under 100." The deterministic incremental cost-effectiveness ratio comes out to 3,165.41, matching "around 3,200 per quality-adjusted life-year." +- **`POPULATION` and `INITIAL_INFECTIOUS` are correctly lifted to named constants** near the top of the first code block, exactly as the guide asks. +- **The "Cost-effectiveness acceptability curve (CEAC)" heading is not a style violation.** The guide gives this exact heading as its own example of a named-quantity noun phrase that overrides gerund-parallelism ("a noun-phrase heading for a named quantity ... is not an editing seam, even among gerund headings"). The apparent gerund/noun-phrase mix a reviewer might flag is explicitly sanctioned, not an inconsistency. + +## Weaknesses and gaps + +1. **The willingness-to-pay threshold used for the acceptability curve does not match the one used two blocks later for value of information, and this is a regression from the source script.** The tutorial plots the curve over `np.linspace(0, 30_000, 61)` but then computes `evpi(outcomes, 50_000.0)` and `evppi_ranking(outcomes, draws, 50_000.0)`, a threshold the CEAC plot never shows. `examples/seir_vaccination.py`, the script this page says it walks through, defines `WTP = 50_000.0` once and uses it everywhere, including a CEAC grid of `np.linspace(0, 100_000, 41)` that comfortably covers it. The qmd both dropped the named constant and narrowed the plotted range below the threshold it uses right after, with no sentence explaining either choice. +2. **`WTP`, the seed, and `HORIZON` are hardcoded instead of lifted to named constants, unlike the source script and unlike sibling tutorials.** `examples/seir_vaccination.py` defines `HORIZON = 10.0` and `WTP = 50_000.0` at module level; the qmd inlines `horizon=10.0` in the `ODEModel` call and writes the literal `50_000.0` three separate times (in the EVPI print string, the `evpi` call, and the `evppi_ranking` call). `des.qmd` lifts `HORIZON`, and `full-pipeline.qmd`, `microsim.qmd`, and `des.qmd` all lift `WTP`. A reader adapting this tutorial to their own threshold has to find and edit three literals instead of one constant. +3. **Neither the EVPI nor the EVPPI output is interpreted.** "The expected value of perfect information (EVPI) puts a monetary value on resolving the remaining uncertainty at the willingness-to-pay threshold" and "`evppi_ranking` attributes that value to individual parameters, so a study can target whichever one the decision is most sensitive to" both state what the function does, but neither output that follows gets a sentence saying what its number means for this decision. Contrast `des.qmd`'s "Service time leads the value-of-information ranking: it sets how fast the queue clears, so it determines whether the extra capacity is worth its cost," which does exactly this for its own model. Here, the EVPPI ranking would show `R0` far ahead of every other parameter, the single most decision-relevant fact in the whole page, and the page never says so. +4. **A numerically odd result sits in the page's own output and is never addressed.** Rerunning the tutorial's exact code (`SeedManager(20260711)`, 1,000 draws) gives EVPI at a willingness-to-pay of 50,000 of about 1,938,103, but the EVPPI ranking's top entry, `R0`, comes out to about 2,358,086, exceeding the EVPI. The expected value of partial perfect information for one parameter cannot exceed the expected value of perfect information for all of them; the gap here is finite-sample noise in the regression-based EVPPI estimator at 1,000 draws, not a real violation, but a reader who has read `voi.qmd`'s inequality would notice the two printed numbers contradict it, and the page gives no warning or explanation. +5. **The page is a navigational dead end.** It ends with "The ordinary differential equation engine adds transmission dynamics to the set of model engines supported by `heormodel`. It returns the same `Outcomes` structure, so the cost-effectiveness and value-of-information analysis reuses the same code as the other engines," a closing summary that restates rather than points forward, with no "Next" line. Every other model-engine tutorial ends with a forward pointer: `mdm-cohort.qmd` links to `voi.qmd`, `microsim.qmd` and `markov-vs-microsim.qmd` each carry an explicit "Next:", and even `des.qmd`'s "Next" (which incorrectly loops back to `full-pipeline.qmd`) at least points somewhere. No tutorial's forward chain reaches this page at all: `des.qmd` is the one page that should point here and instead points at `full-pipeline.qmd`, so a reader following "Next" links in order never arrives at the compartmental model. +6. **The opening paragraph does not situate the page relative to the tutorials before it**, unlike its immediate predecessors. `microsim.qmd` opens by noting it "runs the same `run_psa`, cost-effectiveness, and value-of-information calls as the [Markov cohort model tutorial](mdm-cohort.qmd), so the two are worth reading as a pair," and `des.qmd` opens by naming what `microsim.qmd` contributes that it builds on. `seir-vaccination.qmd`'s opening states what the tutorial teaches and why the engine is needed, but never links back to a prior tutorial or says where this page sits in the model-engines group. +7. **The companion notebook was never executed and its prose has drifted from the page, with two literal typos.** `docs/_notebooks/seir-vaccination.ipynb` has no `execution_count` and no output on any of its 8 code cells, confirmed by inspecting the notebook JSON, so the guide's mandatory render-and-check step has not been done. Its markdown cells are also an earlier, uncorrected draft: cell 12 reads "All value of information functinos work with this moodel's output, including `evpi`:" (two misspellings, "functinos" and "moodel's") and cell 14 reads "And `evpp_ranking`:" (missing an "i"). Cell 8's prose ("Like with the other models, we define the parameter set, take a sample from those, generate outcomes with `run_psa` and then we can get our icer table:") is markedly more casual than the qmd's corresponding sentence and would fail the style guide's methods-appendix voice on its own. +8. **No herd-immunity intuition ties the vaccination rate to the near-elimination result.** The page shows that `nu=1.0` cuts total infections from about 73,000 to under 100 but never explains why, in terms a reader could reuse: a reproduction number of 1.8 needs roughly 44% of the population immune (1 - 1/R0) to stop sustained transmission, and a vaccination rate that outpaces the epidemic's own growth reaches that threshold before the outbreak takes hold. Without this, a reader adapting the model to a different R0 or vaccination rate has no way to predict whether their own scenario will near-eliminate the disease or merely blunt it. + +## Writing-style issues (with quoted lines) + +- **"so an epidemic feeds on itself"** is a figure of speech that personifies the epidemic; the style guide's anthropomorphizing section rules out exactly this register ("a model does not want ... describe what the code computes"). Here nothing computes or wants; the plain content is that the force of infection rises with prevalence, which is stated in the same sentence already ("It is proportional to the current infectious prevalence"), so the phrase adds a figure of speech without adding information. +- **The closing paragraph is a banned closing summary.** "The ordinary differential equation engine adds transmission dynamics to the set of model engines supported by `heormodel`. It returns the same `Outcomes` structure, so the cost-effectiveness and value-of-information analysis reuses the same code as the other engines" restates what the page already showed rather than pointing the reader anywhere, which the guide's Tone section rules out ("no closing summaries that restate the section"). +- No em-dash, exclamation mark, or banned-vocabulary (leverage, comprehensive, robust, streamline, and so on) violation was found in the qmd itself. +- Every acronym the page actually writes out is spelled out on first use: "quality-adjusted life-years," "incremental cost-effectiveness ratio," and "expected value of perfect information (EVPI)" all appear in full before any bare acronym. "CEAC" appears once, in the heading's parenthetical, and the guide's rule ("with the acronym in parentheses only if the page uses it again") is not quite met: the page never uses "CEAC" again, so the parenthetical is technically unearned, though this is a minor point next to the heading itself being sanctioned. +- Headings are gerund-form and parallel except the one sanctioned noun phrase (see Strengths): "Specifying the compartments and their dynamics," "Reading the epidemic curves," "Computing the base-case cost-effectiveness results," "Running the probabilistic sensitivity analysis," "Cost-effectiveness acceptability curve (CEAC)." +- The notebook's typos and casual phrasing are quoted under Weaknesses item 7 rather than repeated here; they are a notebook-only problem, not present in the qmd. + +## Concept introduction and sequencing + +- The force of infection, the R0-to-beta relationship, the two reward channels, and the trajectory-inspection step are all introduced with a full sentence before the code that uses them, and each is interpreted after its output; this is the strongest part of the page and matches what `mdm-cohort.qmd` and `microsim.qmd` do at their own introduction points. +- Using `evpi` and `evppi_ranking` before `voi.qmd` formally names and derives them is consistent with every other model-engine tutorial, not a flaw unique to this page: `full-pipeline.qmd`, the very first tutorial in the whole sequence, already introduces EVPI with a one-sentence definition and never spells out EVPPI by name either, deferring both to `voi.qmd`. This page follows the same pattern (EVPI is spelled out once; EVPPI is only ever named through the function `evppi_ranking`), so it is no more exposed than its predecessors. +- As the fifth and last model-engine tutorial, this page should be the hinge into "Sensitivity and value of information," the next group in `index.qmd`. It is not: no "Next" line exists, and `des.qmd`, which should hand off here, instead loops back to `full-pipeline.qmd`. Fixing `des.qmd`'s link without also adding one here would still leave a reader following "Next" through the model-engines group stranded at `des.qmd`, since nothing points to this page from anywhere in the linked chain. +- The opening paragraph also does not link backward, so a reader arriving here directly (rather than by clicking "Next" from `des.qmd`, which is impossible today) gets no orientation to what the other four engine tutorials already covered. + +## Prioritized recommendations + +**Major** + +1. Fix the "Next" chain: change `des.qmd`'s "Next" link to point here, and add a "Next" line at the end of this page pointing into `dsa.qmd` (the first tutorial in "Sensitivity and value of information"). As written, no forward path through the model-engines tutorials ever reaches this page. +2. Reconcile the CEAC threshold range with the EVPI and EVPPI threshold: either widen `np.linspace(0, 30_000, 61)` to cover 50,000, or lower the EVPI and EVPPI calls to a threshold the plotted curve actually shows, and restore a single named `WTP` constant (matching `examples/seir_vaccination.py`'s `WTP = 50_000.0`) so the three uses cannot drift apart again. +3. Add a sentence interpreting the EVPPI ranking's output, at minimum that `R0` leads the ranking by a wide margin and why that follows from its role in the force of infection, matching the interpretive sentence every sibling engine tutorial gives its own value-of-information output. +4. Render `docs/_notebooks/seir-vaccination.ipynb`, fix its two typos ("functinos," "moodel's," "evpp_ranking"), and bring its prose in line with the qmd's current wording; the render-and-check step required by the style guide has not been done for this notebook. +5. Lift `HORIZON` and the sample size for the probabilistic sensitivity analysis (currently the bare literal `1000`) to named constants alongside `POPULATION` and `INITIAL_INFECTIOUS`, matching `examples/seir_vaccination.py`'s own practice. + +**Minor** + +6. Replace "so an epidemic feeds on itself" with a plain restatement of the mechanism already given in the same sentence, or cut it. +7. Replace the closing paragraph's restatement with either a genuine forward pointer (see recommendation 1) or delete it if the "Next" line alone suffices. +8. Add one sentence on the herd-immunity threshold (roughly 1 - 1/R0) to explain, in terms a reader can reuse, why a vaccination rate of `nu=1.0` against `R0=1.8` nearly eliminates transmission. +9. Add a sentence to the opening paragraph linking back to at least one prior engine tutorial, matching the pattern in `microsim.qmd` and `des.qmd`. +10. Either use "CEAC" again somewhere in the prose after the heading introduces it, or drop the parenthetical acronym from the heading. diff --git a/devdocs/tutorial-evaluation/eval-voi.md b/devdocs/tutorial-evaluation/eval-voi.md new file mode 100644 index 0000000..e1e04cf --- /dev/null +++ b/devdocs/tutorial-evaluation/eval-voi.md @@ -0,0 +1,51 @@ +# Evaluation: Value of information (`voi.qmd`) + +**Slot:** Sensitivity and value of information (2 of 2) + +**Overall verdict:** Every number and ranking I reran matched the prose exactly, including the two figures, and the expected value of sample information (EVSI) and expected net benefit of sampling (ENBS) sections are the page's real contribution: a genuinely new, well-motivated worked example that goes beyond the six earlier, brief uses of the expected value of perfect information (EVPI) and expected value of partial perfect information (EVPPI). But this page is positioned as the definitive value-of-information tutorial for a workflow the reader has already run, uninterpreted, in `full-pipeline.qmd` (tutorial 1) and interpreted with its full formula in `byo-outputs.qmd` (tutorial 2), and it never acknowledges that history or the weaker, formula-free EVPI treatment it gives here by comparison. It also ends the page with no interpreting sentence after its last plot and no "Next" link, and its own predecessor's "Next" link skips it entirely. Rating: **Good**. + +## Strengths + +- **Every reproducible claim is correct.** I installed the package with `uv` and reran all seven code blocks: EVPI at willingness-to-pay 100,000 is 2,802.5, and the EVPPI ranking is `u_S1` 823.4, `c_trtA` 366.8, `c_trtB` 66.0, then a sharp drop to single digits, exactly the "dominates the ranking... two treatment costs follow... everything else contributing almost nothing" the prose claims. The EVSI fan is near zero through willingness-to-pay 80,000-90,000 at every sample size from 25 to 6,400 and peaks at exactly the 120,000 grid point, matching "small below about 90,000... peak sharply near 120,000," and the ENBS optimum I computed is 1,600 patients, matching the page's own printed `best`. +- **The EVSI and ENBS sections are a real, adaptable worked example**, not a synthetic construct built to check an analytic answer. `PATIENT_SD = 0.20 # per-patient sd of a preference-based utility instrument` and the survey-size list are named constants a reader can edit directly for their own instrument and study design, and the fan-of-curves plot is an effective, uncommon way to show how a survey's value approaches its ceiling as sample size grows. +- **The rhetorical question is used exactly as the style guide allows**: "How many patients should the survey enroll? The expected net benefit of sampling (ENBS) answers that by multiplying the per-person EVSI by the population it would inform..." poses a question and answers it in the same breath, the one sanctioned use. +- **Mechanically clean.** No em-dashes, no exclamation marks, no banned filler words (leverage, delve, comprehensive, robust, streamline, and the rest), `WTP` is always spelled out as "willingness-to-pay threshold" in prose and never used as a bare acronym outside code, and the "Calculating the expected value of perfect information (EVPI)" heading matches the style guide's own worked example for a named-quantity heading almost verbatim. +- **The EVPPI formula is given in full**, $\text{EVPPI}_\phi = \mathbb{E}_\phi[\max_d \mathbb{E}_{\theta\mid\phi}\,\text{NMB}_d] - \max_d \mathbb{E}_\theta\,\text{NMB}_d$, where NMB is net monetary benefit, with a plain-language reading before it ("It replaces the inner maximization of EVPI with an expectation taken over only the parameter of interest"), which is exactly the introduce-before-use, interpret-after pattern the guide asks for. + +## Weaknesses and gaps + +1. **The EVPI treatment on this page is thinner than the one a reader already saw in `byo-outputs.qmd` (tutorial 2).** `byo-outputs.qmd` gives EVPI's full formula, $\text{EVPI} = \mathbb{E}_\theta[\max_d \text{NMB}_d(\theta)] - \max_d \mathbb{E}_\theta[\text{NMB}_d(\theta)]$, and a sentence of intuition. `voi.qmd`, arriving eight tutorials later and positioned as the value-of-information tutorial, gives only "EVPI is the difference between a decision made with perfect information and one made with current information, an upper bound on the value of any research," no formula at all. A reader who skipped straight to this page for the definitive treatment gets a weaker one than the page that used EVPI in passing. +2. **`NMB` is used but never spelled out on this page.** The EVPPI formula's $\text{NMB}_d$ is the only place net monetary benefit appears in `voi.qmd`; the term "net monetary benefit" and its own formula ($\lambda E_d(\theta) - C_d(\theta)$, given in `byo-outputs.qmd`) never appear here. The style guide requires spelling out every acronym on first use *per page*; a reader who opens this page on its own (it advertises "so this page runs on its own") meets `NMB_d` cold. +3. **The claim that `u_S1` EVPPI "tracks EVPI closely here" does not hold across the range shown in the figure.** I computed both curves on the same 21-point grid the page plots: at willingness-to-pay 80,000, EVPI is 5,089.6, a local bump comparable in size to its eventual peak (10,083.6 at 120,000), while EVPPI for `u_S1` at that same point is 3.9, essentially zero, and checking `evppi_ranking` at 80,000 shows `c_trtB` (3,214), not `u_S1`, driving that bump. The prose's "tracks EVPI closely here" is true near the peak (100,000-130,000), but the black EVPI line in the rendered figure will visibly bulge around 70,000-90,000 in a way the orange `u_S1` EVPPI line does not follow, and nothing in the text prepares the reader for that discrepancy or explains it comes from a different parameter. +4. **The Gaussian-process metamodel gets a six-word aside with no explanation and no link**, in sharp contrast to how the same method is treated two tutorials later in the sequence. `voi.qmd`: "`method="gp"` (a Gaussian process) suits groups of many parameters evaluated jointly." Compare `surrogate-calibration.qmd`: "A Gaussian process is used because it is fast to fit and reports its own uncertainty, so a draw from it carries the approximation error forward... For the full treatment, see the [GP regression chapter](https://bookdown.org/rbg/surrogates/chap5.html)." The style guide's rule ("name the method, say in a sentence or two what it does and why it is used here, and link to a reputable external page") is followed generously elsewhere and skipped here, and the `spline` metamodel gets no explanation anywhere on the page or site. +5. **The ENBS section's decision inputs are inlined, unlike every other decision input on the page.** `WTP`, `PATIENT_SD`, and the survey-size list are named constants a reader can find and edit. The population size, discount rate, time horizon, and survey cost function are not: `beneficiaries = 2_000 * (1.03 ** -years).sum()` and `cost = 100_000 + 300 * np.array(sizes)` bury four decision inputs (2,000 patients a year, a 3% discount rate, a 10-year horizon, a 100,000 fixed cost, a 300 per-patient cost) as bare literals inside one expression each. A reader adapting the ENBS calculation to their own study has to find and edit five numbers hidden inside two lines of arithmetic, exactly the friction the style guide's "lift decision inputs to named constants" rule exists to prevent. +6. **The page ends with no interpreting sentence and no "Next" link.** The last content on the page is a bare code block plotting the ENBS curve with a red star at the optimum; nothing follows it. Every other tutorial checked against this one (`byo-outputs.qmd`, `des.qmd`, `dsa.qmd`, `full-pipeline.qmd`, `microsim.qmd`, `parameter-inputs.qmd`, and more) ends with a sentence pointing to what comes next. `voi.qmd` is the only one in the sequence that has neither. + +## Writing-style issues (with quoted lines) + +- **Opening is a list of acronyms, not a statement of what the reader can then do.** "This tutorial shows how to run the full value-of-information workflow: expected value of perfect information (EVPI), of partial perfect information (EVPPI), and of sample information (EVSI), on the Sick-Sicker cohort state-transition model." This names four things the code computes, not the decision the reader will be able to make afterward (whether to fund a study, and at what size), which is exactly the "inventory of function calls" pattern the guide asks pages to avoid in favor of naming the goal. +- **Headings are mostly parallel gerunds** ("Specifying the decision model," "Calculating the expected value of perfect information (EVPI)," "Ranking parameters with EVPPI") but the last two sections switch to bare named-quantity nouns, "Expected value of sample information (EVSI)" and "Expected net benefit of sampling (ENBS)." The style guide explicitly endorses a noun-phrase heading for a named quantity, so this is not a defect, but the page is inconsistent about which form it uses for the same kind of section (EVPI gets "Calculating the expected value of..."; EVSI does not). +- **The local variable `evsi` does not literally shadow an import**, though the name suggests it might: `heormodel.voi` exports `evsi_regression`, `evsi_moment_matching`, and `evsi_importance_sampling`, never a bare `evsi`, so `evsi = pd.Series({n: fan[n][i_wtp] for n in sizes})` does not collide with anything imported on the page. It is still a confusing choice next to the imported `evsi_regression`, and the same naming appears in `examples/voi_tutorial.py`, so it is a house convention rather than a one-off slip. +- Punctuation and vocabulary are clean throughout: no em-dashes, no exclamation marks, no banned filler words, sentence-case headings, backticks used consistently for identifiers (`u_S1`, `evppi_ranking`, `evsi_regression`). + +## Concept introduction and sequencing + +- **EVPI and EVPPI are used, with numbers and interpretation, in six tutorials before this one formally introduces them: `full-pipeline.qmd` (tutorial 1 of the whole site), `byo-outputs.qmd` (tutorial 2, which gives EVPI's formula), `mdm-cohort.qmd`, `microsim.qmd`, `des.qmd`, and `seir-vaccination.qmd`.** By the time a reader following the sequence in order reaches `voi.qmd` at position 10, they have already run `evpi()` and `evppi_ranking()` four to six times with correct interpretations attached each time, but `voi.qmd` never says so and its opening treats both quantities as introduced here for the first time. One added sentence, such as "you have already used EVPI and EVPPI in the model-engine tutorials; this page gives the definition behind those calls and extends the workflow to EVSI and the size of a study worth funding," would resolve the mismatch between the page's implicit claim to be the first treatment and the fact that it is not. +- **Two of the six early uses explicitly promise a forward link to this page** (`mdm-cohort.qmd`: "The value of information tutorial takes this same model further"; `full-pipeline.qmd`: "Next: the value of information tutorial takes the cohort state-transition model... through the expected value of perfect, partial perfect, and sample information"), so the site's own design intends `voi.qmd` to be a payoff the reader has been promised, not a stand-alone late entry. It delivers on that promise for EVSI and ENBS; for EVPI it delivers less than promised, for the reason given in weakness 1 above. +- **`dsa.qmd`, this page's immediate predecessor in the sequence, does not link forward to it.** `dsa.qmd` ends: "Next: the [replication gallery](replication-gallery.qmd) collects the published models these engines reproduce," skipping past `voi.qmd`, the very next item in the same "Sensitivity and value of information" section of `index.qmd`, and past the entire calibration section besides. A reader clicking "Next" through the site rather than using the sidebar never reaches `voi.qmd` at all. +- **Reconstructing the full cohort model is justified, not bloat.** The page states its reason directly: "the code below reconstructs both so this page runs on its own," and the same pattern appears in the sibling `dsa.qmd`. Given the Colab badge at the top of the page, each tutorial needs to run standalone in a fresh notebook, so duplicating roughly 40 lines of model-building code the reader has already seen in `mdm-cohort.qmd` is a reasonable trade for that requirement, and the page states the trade-off rather than leaving the reader to guess at it. + +## Prioritized recommendations + +**Major** + +1. Add a sentence to the opening (or to the "Calculating the expected value of perfect information (EVPI)" section) that acknowledges EVPI and EVPPI were already used, with numbers, in the model-engine tutorials, and states what this page adds: the formal EVPPI derivation, EVSI, and ENBS. This is the single highest-value fix: it turns an apparent gap in the page's self-awareness into an explicit, correct statement of its place in the sequence. +2. Either add the EVPI formula to this page (it is one line, already written in `byo-outputs.qmd`) or spell out net monetary benefit and its formula before using `NMB_d` in the EVPPI equation; right now the page's own EVPI section is less rigorous than the earlier page that used EVPI only in passing. +3. Add a closing sentence after the final ENBS plot and a "Next" link to the first calibration tutorial (`calibration-workflow.qmd`, per `index.qmd`'s sequence), matching every other tutorial checked. Separately, fix `dsa.qmd`'s "Next" link to point here instead of skipping to the replication gallery (a `dsa.qmd`-side fix, not a `voi.qmd`-side one, but the two pages should be fixed together). +4. Qualify or correct "tracks EVPI closely here": the EVPI curve has a local bump around willingness-to-pay 70,000-90,000 (driven by `c_trtB`, not `u_S1`) that the plotted `u_S1` EVPPI curve does not follow. Either narrow the claim to the region near the peak or add a sentence noting that a different parameter accounts for the earlier bump. + +**Minor** + +5. Give the Gaussian-process metamodel the same one-to-two-sentence explanation plus external link that `surrogate-calibration.qmd` and `calibrate-microsim.qmd` already give it, rather than a six-word parenthetical; add at least a one-clause explanation of what the default `spline` metamodel does. +6. Lift the ENBS section's population size, discount rate, time horizon, and survey cost parameters (2,000 patients a year, 3% discount rate, 10-year horizon, 100,000 fixed plus 300 per patient) to named constants alongside `WTP` and `PATIENT_SD`, rather than as bare literals inside `beneficiaries = 2_000 * (1.03 ** -years).sum()` and `cost = 100_000 + 300 * np.array(sizes)`. +7. Pick one heading style for a section that introduces a named quantity and use it for both EVPI and EVSI: either "Calculating the expected value of sample information (EVSI)" to match "Calculating the expected value of perfect information (EVPI)," or drop "Calculating" from the EVPI heading to match the bare noun-phrase form used for EVSI and ENBS.