DO NOT MERGE — throwaway: confirm the CI job conditions fire on a master PR - #340
Closed
derrynknife wants to merge 15 commits into
Closed
DO NOT MERGE — throwaway: confirm the CI job conditions fire on a master PR#340derrynknife wants to merge 15 commits into
derrynknife wants to merge 15 commits into
Conversation
Turnbull.fit defaults to turnbull_estimator="Fleming-Harrington" while KaplanMeier.fit is KM. The EM recovers the same r and d either way; the three options differ only in how those become a survival curve. So comparing a default Turnbull fit against KaplanMeier and reading the gap as a defect is an easy mistake, and the docstring gave no hint of it. It is the mistake #260 was filed on, and the mistake made again while checking whether #260 was still open — twice is enough to write it down. On x=[2,3,3,4,5,6], tl=[0,0,1,1,2,2] the survival at 2 is 0.750 under KM, 0.765 under FH, 0.779 under NA. Matched, Turnbull agrees with KaplanMeier to ~1e-9 on sf and cb across right-censored and left-truncated data. Only KM is the NPMLE. Maximising the truncated likelihood directly over the mass vector gives 0.750; FH's 0.765 scores worse on that same likelihood, which is what an exp(-H) construction should do. FH is the default for tail and zero-inflation behaviour (v0.8.0), not because it maximises anything. The new test pins the three figures and the NPMLE identity against a brute-force Nelder-Mead maximisation, so the docstring cannot quietly stop being true. No behaviour change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
The local check was piped through tail, so the pipeline reported tail's exit status and the failure was invisible. Exactly the pipefail hazard described a few commits ago; running black with its own exit status surfaces it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
…uatqe Say why a Turnbull fit does not equal a Kaplan-Meier fit
pytest --doctest-modules over distributions/ gives 39 failures. Thirty are the numpy-2 scalar repr and are cosmetic. Nine were not. Uniform.ff's example called Uniform.sf, and ExpoWeibull.cs's called ExpoWeibull.sf. In both the printed values were correct for the function being documented and wrong for the one being called, so each example read as though the two functions agreed. LogLogistic.sf carried values from some other parameterisation (0.622 where the answer is 0.988), LogLogistic.mean(3, 4) claimed 3 against 3.3322 — the closed form is alpha (pi/beta) / sin(pi/beta) — and Exponential.qf had stale digits. The remaining four were the CustomDistribution Gompertz walkthrough, whose multi-line def used >>> where doctest needs ..., so pasting it raised IndentationError. In every case the code was right and the docs wrong, which is the reassuring direction, but a reader checking their understanding against them would have been misled. They accumulated because the doctests are not run; whether to run them in CI is a separate style decision and is left alone here. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
Gamma.fit(x, how="MPP") now raises, joining Beta and ExpoWeibull which already declined for the same reason. A probability plot rearranges the survival function so a transform of the data falls on a straight line. Weibull gives log(-log S) = beta log x - beta log alpha: the axes do not depend on the answer. The Gamma has no such rearrangement — its CDF is the regularised incomplete gamma and the shape sits inside that special function rather than outside as an exponent. The only straight-line y-axis is the inverse incomplete gamma, which needs the shape. To draw the axis you need the answer; to get the answer you need the axis. The code broke the circle by guessing the shape from moments, drawing the plot on the guess and regressing. A wrong guess means a wrong axis, points that are no longer straight on it, and a line fitted through a curve — a confident wrong estimate rather than an error. An offset made it worse, since the shift distorts the low-x end hardest and that is where the shape information is. plot() is untouched: it transforms with the fitted parameters, so the axis is correct by the time it is drawn. MLE, MSE and MOM are unchanged, offset included. Deleting the 118-line Gamma.mpp override takes the rr="x" mis-inversion and the censored-data LinAlgError from #257 with it, by making both unreachable. The three tests that asserted Gamma MPP recovery now assert the refusal and keep their offset-recovery coverage under MLE, and the test_fit.py MPP sweep gates on supports_mpp rather than a hardcoded list so a new distribution will not need it edited. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
pytest --doctest-modules over distributions/ is now green: 139 examples, no failures, down from 39 failing. Most were the numpy 2 scalar repr. Weibull.mean(3, 4) prints np.float64(2.7192074311664314); the docstring recorded the bare float numpy 1 used to print. The examples now carry the wrapper because that is what a reader sees at their own prompt. The alternative was np.set_printoptions(legacy="1.25") in a fixture, which keeps the docstrings prettier by showing people output their session will not produce — prettier, but not true. Four qf examples exceeded 79 columns once the real output was recorded, numpy having rewrapped the arrays differently from the hand-wrapping. Rather than reflow them into something numpy would never emit, those examples now take fewer probabilities, so what is printed is exactly what that input produces. Two scalar examples had drifted in the last digit and are re-recorded against a direct run. Every rewrite was gated on the numbers agreeing to 1e-12 first, so a genuine mismatch could not be blessed by the sweep; nothing failed that gate. Two files needed hand correction afterwards where mean and moment share a value and the text search matched the wrong occurrence. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
A docstring example is a promise about what the library prints, and it
is the first thing a user or a coding agent reaches for -- help() is
faster than opening the docs. Nothing was checking it, so it drifted.
Turning on --doctest-modules over the package gave 59 failing tests;
this fixes all of them and adds the step to the deployment workflow.
Beyond the cosmetic drift (numpy 2 scalar reprs, optimiser output from
two rewrites ago), the run found:
- Twelve examples that could not run at all. Six regression
docstrings (PH, AH, PO, AFT, AcceleratedLife, Frailty) were
sketches -- `model = PH(Weibull).fit(x, Z=covariates, c=c)` with
none of x, covariates or c ever defined. Four used >>> on the
continuation lines of a multi-line call, so pasting them raised
SyntaxError. plotting_positions imported from a module that moved
several releases ago. ParametricFitter.fit demonstrated how='MPP'
on interval-censored input, which now correctly requires the
Turnbull heuristic and raises without it.
- The five ParametricRegressionModel prediction examples (sf, ff,
df, hf, Hf) had been copied from the univariate Parametric class
and never adapted: they built a Weibull.from_params([10, 3]) and
called it with no covariates, documenting a signature the method
does not have.
- Parametric.var() claimed 11.229 for a Weibull(10, 3). The variance
is 10.533; the code was right.
- Several examples fitted unseeded random data and then recorded
specific digits. They now seed.
Parametric.hf and Parametric.Hf returned a 0-d array for scalar input
where sf, ff, df and qf returned a numpy scalar, as did cs -- against
their own Returns sections, which promise "the scalar value ... if a
scalar was passed". np.where does not collapse a 0-d result; [()]
does, and is a no-op on a real array.
Two doctest flags are set in pyproject.toml. NORMALIZE_WHITESPACE,
because numpy picks its own line breaks and column padding for an
array and both move with the widest element. ELLIPSIS, so an example
ending in a fit can write 529.05371... rather than all seventeen
digits: the trailing digits of an optimiser's output are not part of
what the docstring promises, and they move with the BLAS and the
platform. Array reprs are left exact -- numpy already prints only
eight significant digits there.
229 examples, all passing. Closes #158.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
CI failed the doctest step on 3.12 and 3.13 while passing on 3.11. The diagnosis was the assumption made in the previous commit: that numpy's eight-significant-digit array repr was tight enough to pin. It is not. The Duane example lands on b = 4.1995e-05 under 3.11 and 4.2032e-05 under 3.12 -- a third-significant-figure difference in an optimiser whose fit is poorly conditioned on ten events. Sixteen of the 229 examples disagree somewhere in their digits between those Pythons. Trimming each documented number back to the digits that agree everywhere would make the docstring show something the reader's own session will not produce, which is what these examples exist to avoid. So the examples record the real output, in full, and the comparison changes instead. conftest.py patches doctest.OutputChecker.check_output with a fallback that runs only after the ordinary text comparison has failed. It fires when the two outputs are identical apart from their numeric literals -- same words, same brackets, same integer-versus-float shape, so "1" never matches "1." and a dtype change is still a failure -- and then compares the numbers pairwise. rel_tol is 1e-3, set by the loosest genuine disagreement observed with no margin beyond it; abs_tol is 1e-12 for a restoration factor whose true value is zero and which surfaces as 1e-16 with whatever mantissa the optimiser stopped on. Patched on the base class rather than installed as a checker: pytest builds its own LiteralsOutputChecker subclass and calls up to this method, so the override survives both plain doctest and pytest without depending on pytest internals. What this forgives is a value drifting inside the tolerance. What it still catches is every defect the sweep found: a stale value from another parameterisation, the wrong function being called, the wrong shape, an exception, a missing import. test_doctest_checker.py pins both halves, using the real output pairs observed on different Pythons -- so the tolerance cannot be widened without a test saying why. ELLIPSIS is dropped from the option flags; it was only there to support the trimmed numbers, and leaving it on invites reintroducing them. Verified against real 3.11, 3.12 and 3.13 interpreters locally: 229 doctests pass on each, and the same 3.12 run shows 16 text-level differences with the fallback disabled. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
The doctest step failed on all three Pythons, on one example: ProportionalIntensityHPP, whose numbers were well inside tolerance. The fallback never got to look at them. Its expected output carries <BLANKLINE> markers, which the text comparison substitutes for empty lines before matching; the fallback did not, so the two skeletons differed on the marker text and it declined before comparing a single number. Why local verification missed it: the fallback only runs once the text comparison has failed, and on this machine that example's fit matches the recorded value exactly. Running against real 3.11, 3.12 and 3.13 interpreters exercised the fallback on the sixteen examples that drift *here* -- and this was not one of them. Passing on three Pythons said nothing about the examples whose numbers agreed on all three. So --doctest-force-numeric is added alongside the fix: it routes every example whose output contains a number through the numeric comparison regardless of whether the text matched, exercising the fallback against all 229 rather than today's accidental few. Outputs with no numbers keep the text comparison; there is nothing in them to compare. CI runs the doctest step a second time under the flag, which costs fifteen seconds and turns "the fallback handles the examples that happen to drift on this machine" into "the fallback handles the examples". The regression test uses the ProportionalIntensityHPP block whole, markers and all, rather than one extracted coefficient -- an extracted line would have gone on passing, since what broke was not a number. Verified on 3.11, 3.12 and 3.13: 229 doctests pass in both modes on each. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
…uatqe Run the docstring examples in CI
`pip install -e ".[docs]"` now installs everything needed to build the documentation, alongside the `tests` extra that was already there. docs/requirements.txt is removed rather than kept alongside the extra: two copies of the same pinned toolchain is the arrangement that drifts, and the repo already settled this pattern for tests, where requirements_dev.txt is `-e .[tests]` plus tools rather than a second pin list. Read the Docs installs the extra directly via extra_requirements, which is their documented form for exactly this; Contributing.rst loses a step. The pins are carried over unchanged, including the ipykernel==6.31.0 cap and the reason for it. matplotlib is not repeated in the extra -- it is a runtime dependency of the package, installed alongside. Verified by a complete `sphinx -b html` in a clean 3.12 virtualenv built only from `pip install ".[docs]"`. Part of #141. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
The offset-threshold section of "Parametric SurPyval Modelling" ran a jupyter-execute cell looping over ['MPP', 'MOM', 'MSE', 'MPS', 'MLE'] for a shifted Gamma. Gamma.fit(how="MPP") now raises, so that cell raised, and since documentation cells execute during the build the whole build failed. Nothing caught it. CI does not build the documentation, and Read the Docs builds only master and tags, so this would have surfaced as a failed hosted build at the next release rather than on the pull request that caused it. It was found by running a build to validate the docs extra. The prose around the cell had gone stale in the same way: it described the multi-start probability-plotting search that the removal deleted, and quoted an MPP tolerance from test_offset_divergence.py that no longer exists. It now explains why the Gamma has no probability plot -- the shape sits inside the regularised incomplete gamma rather than outside as an exponent, so the only straight-line axis is the inverse incomplete gamma, which needs the shape being estimated -- and notes that plot() is unaffected because by then the parameters are known. Verified by a complete build: succeeded, with only the 18 pre-existing warnings (duplicate changelog labels, the rtd-theme deprecation, and the ProportionalIntensityNHPP_ autodoc imports). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
The docs execute every `.. jupyter-execute::` cell as they build, so they are a second test suite that exercises the public API for real -- and one that a change touching no documentation file can break. Removing Gamma's probability-plot fitting did exactly that, and nothing noticed, because Read the Docs builds only master and tags: it would have surfaced as a broken hosted build after the release rather than on the pull request that caused it. The job is conditioned on `github.base_ref == 'master'`, which is set only for pull_request events, so it runs on the develop -> master release pull request and nowhere else. Deliberately not on pushes to master: Read the Docs rebuilds there anyway, and by then the gate has nothing left to gate. It matches .readthedocs.yaml rather than the test jobs -- Python 3.12, the package installed via its own `docs` extra -- because the point is to reproduce the hosted build, and it uploads the rendered HTML as an artifact for review on the release PR. Not built with -W. There are 18 pre-existing warnings, mostly duplicate labels from autosectionlabel meeting the changelog's repeated section headings; clearing those and then failing on warning here and in .readthedocs.yaml together is a separate change, and turning it on before then would fail every release. The residual gap is deliberate and now documented: a break introduced on a pull request into develop is caught when the release is prepared, not when it lands. Building on every pull request would cost minutes on each, and a path filter would not have helped here -- the change that broke the build was in gamma.py, not under docs/. Contributing.rst claimed the documentation build already ran once per pull request. It did not run in CI at all. It now describes what runs where, and names the trade-off. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
Pull requests into `develop` now run lint alone -- about a minute, against the nine the suite takes across three interpreters. The suite still runs in full on the release pull request into `master` and on pushes to `master`. The reason is the edit-review loop. With a single maintainer running the suite locally before pushing, the pull-request run was mostly confirming what was already known, while being the slowest part of working on the package. What this gives up is real and is written down rather than glossed: a failure that appears on only one interpreter is now found when the release is prepared, with a release's worth of commits to search rather than one. That is not hypothetical -- the doctest numeric comparison landed green on 3.11 and failed on 3.12 and 3.13, and it was the pull-request run that caught it. Contributing.rst gains a table of which jobs run on which event, the timings that motivate the split, and what to run locally to compensate: the suite across more than one interpreter when touching numerics, and a docs build when changing the behaviour of a public function. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
With the test suite no longer running on pull requests into `develop`, this is the other half of that trade: one command runs the suite and both doctest passes on 3.11, 3.12 and 3.13, and refuses to say "passed" unless all of them did. The failure it guards against is not hypothetical. The doctest numeric comparison passed on 3.11 -- the interpreter it was written on -- and failed on 3.12 and 3.13, because an optimiser landed on a different last digit. Nothing short of running the other interpreters finds that. Environments live in a git-ignored .venvs/ and are reused, so only the first run pays for the installs. uv is used when available and it falls back to venv and pip when not; an interpreter that is not installed is reported rather than fatal. Lint is deliberately absent -- it runs on every pull request already, so it is not what this is for. The command list is a copy of the workflow's, with a comment saying so: if the two drift this stops being a preview of CI and becomes its own thing that can pass while CI fails. Contributing.rst names it as the compensation for the CI split rather than leaving "run it locally across interpreters" as advice with no mechanism. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Throwaway. Do not merge. Will be closed as soon as the checks report.
#339 adds two job conditions to
.github/workflows/actions.yml:#339 targets
develop, so its own run only proves the skip half — both jobs correctly reportedskippedthere. Whether they actually fire on a pull request intomasteris unproven, and the next thing to find out would otherwise be the real release.This PR exists only to answer that. Expected on this run:
lint— successsurpyval_ci— runs across 3.11 / 3.12 / 3.13 (it skipped on the develop PR)docs— runs, builds the documentation and uploads the HTML artifact (it skipped on the develop PR)The branch is #339's head, so this is also a dry run of what the eventual release pull request will do. It is a draft and will be closed once the checks report; nothing here is intended to reach
masterby this route.🤖 Generated with Claude Code
https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
Generated by Claude Code