Type-hint ratchet, an ArrayBox stub, and the API bugs turning mypy back on found - #339
Merged
Merged
Conversation
`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
derrynknife
force-pushed
the
claude/surpyval-next-steps-7uatqe
branch
from
August 5, 2026 00:00
ab5e09f to
d4cd31b
Compare
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
The multivariate copulas and the beta survival tree and forest had no autodoc coverage at all -- their only mentions in the documentation were narrative prose and, in the forest's case, a `:mod:` cross-reference that resolved to no page. The degradation page stopped at the path models. This is the second half of #141. New pages: surpyval.multivariate the Copula base, the five copula classes, CopulaModel, MultivariateSurpyvalData surpyval.beta RandomSurvivalForest, SurvivalTree, and the node classes a serialised tree is built from The degradation page gains the Wiener and gamma stochastic-process models, ProcessRUL, and destructive degradation. `_bounds` and `population` are deliberately left out: neither is exported from surpyval.degradation's `__init__`, so both are internal helpers rather than public API. Two surfaces the issue did not name but which match its description -- "newer public surfaces that only have narrative examples today" -- also had no autodoc, and now have pages: shared frailty and Buckley-James. Each explains what the model is for rather than only listing methods. surpyval.regression carried two headings, "Accelerated Time Models" and "Accelerated Life Models", with nothing underneath them; they rendered as empty sections while that content sat in regression/parametric. The page is reorganised into semi-parametric, parametric and correlated-observations. Three automethod directives on the NHPP regression page pointed at cif, iif and inv_cif on the fitter, which does not have them -- they are on the model the fit returns, and take covariates alongside the time. Those were three of the 18 warnings standing between the docs build and -W. Every autodoc target in the documentation was checked by importing it: 138 targets, 0 failures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
Pointing the NHPP regression page's cif/iif/inv_cif at the model class fixed the three broken autodoc targets, but documented those methods a second time -- they already appear on the proportional-intensity model page -- which Sphinx reports as `duplicate object description`. Three warnings out, two warnings in, for no net gain. The page now links to them with `:meth:` roles instead. One canonical place per method, references from anywhere else, which is the structure the rest of the documentation already uses. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
Three of the four warning classes the build emits. Each is a real defect, and together they are the noise that hid three broken autodoc targets long enough for nobody to notice the methods were missing from the rendered docs. Duplicate labels (12 of the 16). autosectionlabel mints a cross-reference target from every section heading; prefixing by document stops two pages colliding but not two headings within one page, and the changelog necessarily repeats "Serialisation", "Degradation", "Regression" once per release. autosectionlabel_maxdepth = 1 keeps labels for page titles, which is what a :ref: between pages actually wants, and stops minting them for the subsections beneath. Nothing referenced those subsection labels -- the changelog has no inbound :ref: at all -- so nothing breaks. The sphinx_rtd_theme deprecation. conf.py re-set html_theme and computed html_theme_path from get_html_theme_path() in a not-on-RTD branch. The theme registers itself as an entry point now, so html_theme alone is enough, and the warning's own text says the call is safe to remove. A title-level inconsistency, which docutils reports as CRITICAL rather than WARNING and which would hard-fail a -W build: "Stratified log-rank" was underlined with ~ on a page that uses ^ for level 3, so docutils saw the level jump from 2 to 4. The remaining warning is a documentation cell printing a Turnbull non-convergence to stderr. It is a genuine one -- an example that does not converge -- and is left for a separate change, since the fix is either to the example or to how it is marked, not to the build. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
Both found by building with -W --keep-going, which reports every
warning with its location and fails, rather than by reading a count off
the end of a successful build.
RandomSurvivalForest's docstring was not valid reStructuredText:
"Specs:" ran straight into a bullet list with no blank line, which
docutils reports as "Unexpected indentation". It had presumably been
that way since the class was written; nothing surfaced it until the new
surpyval.beta API page started rendering it. Rewritten as valid RST,
with the "predicition" typo fixed and the bullets ("Each tree is
trained") replaced with what the class actually does.
The other was the interval-censored Turnbull example in the
non-parametric page, whose EM ran out of iterations and printed a
convergence warning to stderr. jupyter-sphinx does not report a file or
line for those, so it was located by reading the executed notebooks it
writes to disk.
The fix there is not `:stderr:`. The fit converges at max_iter=10000 --
the default 1000 simply is not enough for that data, since the EM
converges slowly when many observations are right censored to infinity
and more than half of these are. So the example now passes max_iter and
the prose says why, which is what a reader hitting the same warning in
their own data needs to know. Marking it expected would have hidden a
usable answer.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
-W --keep-going in the CI docs job, and fail_on_warning in .readthedocs.yaml. Both, in one change: with only one of them set, that one goes green while the other publishes a broken page. Contributing.rst documents the same command, so a local build cannot swallow a warning that CI will reject. A sphinx warning is rarely cosmetic. A broken cross-reference renders as plain text; a mistyped autoclass path drops the class from the page entirely; a page in no toctree is published and unreachable. The build reports success and ships something wrong, and the only evidence is a line in a log nobody reads. That is how three broken ProportionalIntensityNHPP autodoc targets survived -- three methods simply absent from the rendered documentation, behind "build succeeded, 18 warnings". Warnings-as-errors only works from a zero baseline, which the preceding commits established: twelve duplicate autosectionlabel labels, the rtd-theme deprecation, a docutils CRITICAL title-level inconsistency, RandomSurvivalForest's unparseable docstring, and a Turnbull example that needed a larger max_iter rather than a :stderr: suppression. --keep-going reports every warning rather than stopping at the first, so a bad build is diagnosed in one pass instead of one per run. Verified with a full local build: "build succeeded", no warnings. One consequence worth knowing: the next hosted build is stricter than any before it. Anything on master carrying a warning this branch does not touch will now fail rather than publish -- the docs job on the release pull request catches that before the tag. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
kind="non-parametric" trees, and any RandomSurvivalForest built from them, selected splits on a statistic wrong by factors of several -- and not merely inflated but reordered. Of the two cases in the issue, the weaker separation scored 1.856 against a true 0.276 while the stronger scored 0.276 against a true 1.225, so trees were choosing the wrong split. The statistic sums over the pooled event times of both children, so the left child's at-risk count is needed at times where the left child has no observation of its own. Those were filled by carrying its risk ladder forward, which was wrong twice: the carried value did not subtract the deaths and censorings that occurred at the time it was carried from, and the tail past the last observation subtracted only deaths, so a child ending in a censored observation kept someone at risk for ever. Both inflate Y_L, biasing the numerator and the variance. Counted directly now: at each pooled time t, the observations with tl < t <= x, which is the (entry, exit] convention xcnt_to_xrd already uses -- so the left child's Y_L and the pooled Y agree on what "at risk" means. Nothing is extrapolated, so the leading and trailing special cases go, and numpy_fill with them. Since tl <= x always holds, those with tl >= t are a subset of those with x >= t, so the count is a difference of two suffix sums rather than an O(N*G) comparison against every grid time -- which matters at a root node with thousands of samples and a candidate split per distinct covariate value. Tested against a deliberately naive implementation of the definition: both issue cases, a censored tail, left truncation, ties shared across children, a child starting after the other's first event, and 200 random partitions. One test pins at_risk_on_grid against xcnt_to_xrd, because a disagreement between them is what makes Y_L / Y stop being a proportion. The existing beta suite passes unchanged, so nothing had been calibrated against the old statistic. Closes #287. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
The package ships py.typed, telling a user's type checker the annotations can be trusted, and mypy runs in CI -- but nothing required an annotation to exist, so mypy checked only the ones that happened to be written. py.typed was a promise kept unevenly, and coverage could regress freely. `disallow_untyped_defs` is now set for surpyval.serialisation, surpyval.metrics and surpyval.univariate.information_criteria. Five functions needed annotating to get there. Deliberately a ratchet, not a target: a module joins the list once it is clean, and from then on an unannotated function in it fails CI. The remaining ~1350 unannotated functions elsewhere do not have to be finished first for the enforced part to start holding. Verified by adding an unannotated function to a listed module and watching mypy exit 1. Turning it on found a real gap. SerialisableMixin.to_json and from_json call self.to_dict() and cls.from_dict(), which the mixin never declares -- every class using it supplies them, but that contract lived only in the docstring, and mypy skips the bodies of unannotated functions, so the calls had never been checked. Declared under TYPE_CHECKING rather than as real stubs: a stub raising NotImplementedError would read better, but it would be inherited, and copula_model decides whether a margin is serialisable with `hasattr(m, "to_dict")`, which an inherited stub would answer True for every time. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
28 functions annotated across 11 files, and surpyval.univariate.nonparametric.* added to the disallow_untyped_defs list: the Kaplan-Meier, Nelson-Aalen, Fleming-Harrington and Turnbull estimators, the log-rank test, and the plotting positions. Verified the same way as the first batch -- an unannotated function added to kaplan_meier.py makes mypy exit 1. Doing it surfaced a load-order fragility. plotting_positions built its ESTIMATOR_FUNCS table from `nonp.nelson_aalen` and its two siblings. Each of those names belongs to both a function and the submodule that defines it, and the package attribute is the function only once the package __init__ has bound it over the submodule -- while that table is built during the __init__, at line 13, relying on the estimators being imported at lines 9-11. It works, but by ordering rather than by design, and mypy resolved the names to the modules and called the table not callable. The three are now imported from their defining modules. The remaining uses of the package namespace in that file are inside function bodies, so they resolve after initialisation has finished and are left as they are. Full suite 2153 passed, doctests 229 passed, mypy/flake8/black/isort clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
surpyval.distribution defines the ABCs every model inherits from -- Distribution, ParametricDistribution, NonParametricDistribution and MultivariateDistribution. Its signatures were already annotated apart from `*args` and `**kwargs`, so all eleven errors were the same one and the fix is `*args: Any, **kwargs: Any` throughout. Small in size but the root of the hierarchy: this is the contract every distribution in the package is measured against, and the first place a user's editor looks when resolving sf/ff/Hf. The return type is left as ArrayLike rather than tightened to NDArray. It is loose for a return, but these are abstract: concrete implementations satisfy them with arrays and with numpy scalars alike, and narrowing the base class would force a choice on all of them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
Eight functions in surpyval.recurrent.nonparametric annotated and the module added to the disallow_untyped_defs list. Annotating them made mypy check their bodies for the first time, which turned up two things. The class sets x, r, d, mcf_hat, var and data on the instance the fit returns rather than in __init__ -- the singleton fitter is called on a bare class and hands back a populated one -- so none of them were declared anywhere. They are now class-level annotations without assignment, which gives them declared types without creating class-level defaults shared between instances. This is the convention #143 asks for. handle_xicn returns a RecurrentEventData or a 4-tuple depending on as_recurrent_data, but said only "one or the other", so every caller taking the default got the union back. Rather than narrow it at this one call site, the two cases are now declared with @overload keyed on Literal[True] / Literal[False]. That is a deliberate step outside this module: there are seventeen call sites, nine of them on the default, and each will meet the same union as the rest of the recurrent package is annotated. Fixing it once beats working around it nine times. Full suite 2153 passed, doctests 229 passed, mypy/black/flake8/isort clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
Eight functions annotated across frailty_fitter.py and the package __init__, and surpyval.univariate.regression.frailty.* added to the disallow_untyped_defs list. Mostly the optimiser's internals: the natural/unconstrained parameter transforms, the finite-difference Hessian, the marginal negative log-likelihood and the two closures the optimiser is handed. `dist` and `distribution` are left as Any, matching what the file already used. Tightening them to ParametricFitter looked obvious and was wrong: the base class does not declare Hf or hf, so mypy rejected `self.dist.Hf(...)` immediately. Those are defined on each concrete distribution rather than on the base -- even though ParametricFitter's own docstring says a distribution "needs only hf and Hf (or sf, ff and df)". Declaring them on the base is the real fix, but it is a change to a class with 25-odd subclasses whose signatures differ, so it does not belong in this commit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
`Fix the last two documentation build warnings` turned a plain reference to SurvivalTree into a :class: cross-reference, which pushed the line to 80 characters. flake8 has failed on it in every CI run since, and because the lint job runs flake8 before mypy, mypy has been skipped rather than run for that whole stretch. Only the wrapping changes; the cross-reference is the same. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
Annotating a function makes mypy check its body, and the last two
ratchet batches annotated parameters as array-like and then indexed,
sliced and divided them. Array-like also covers str, bytes and scalars,
so none of that is valid on it: the signature said one thing and the
body assumed another. mypy said so -- 154 errors across six files --
but flake8 fails ahead of it in the lint job, so the mypy step was
skipped rather than run and none of it reached CI.
turnbull, rank_adjust and NonParametricCounting.from_xrd now take their
arguments as arrays before using them as arrays. turnbull's result
dictionary is declared dict[str, Any]; it carries arrays, the estimator
name and the convergence flags together. _logrank_z_v declared c and n
as arrays while handling None for both internally. And
NonParametricCounting.fit declared windows as array-like when it is the
{item: [(start, end), ...]} dictionary its own docstring describes.
success_run was the same shape of problem in its argument handling:
`if confidence and alpha` is a truthiness test, so passing both with
either set to zero skipped the "only one of" raise, and confidence=0
fell through every branch and left alpha as None. Both are now tested
against None.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
All twelve loaders return a DataFrame and take no arguments, so this is twelve return annotations and surpyval.datasets on the disallow_untyped_defs list. Eight modules enforced. The unused handle_xicn import goes with them. Nothing referenced it, and the F401 per-file ignore for __init__.py meant flake8 would never have said so. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
CI's mypy runs on 3.12 and rejected the assignment; the same mypy on 3.11 accepted it, so a local run on one interpreter is not a proxy for the lint job. The scalar branch is deliberate -- 0.0 broadcasts over the per-interval counts without allocating an array of zeros -- so the declaration says so rather than the else branch changing to match. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
The 25 parametric distributions could not be annotated until there was a way to say what their arguments are, because they deal in two kinds of value and only one can be an autograd box. `Numeric` is what a distribution function is evaluated at -- times, or probabilities for qf. Instrumenting a real fit shows this is an ndarray on every path reached, including the proportional-hazards and accelerated-failure-time regression fits, so it is typed as one. `Boxable` is a parameter, or anything computed from one. Maximum likelihood differentiates these functions, so autograd substitutes an ArrayBox for each parameter to carry the derivative; the same fit shows alpha arriving as float64 and as ArrayBox. A box is neither a float nor an ndarray, so this is deliberately not narrowed to a numpy type. It also means the "array-like in, array out" convention used elsewhere in the package must not be applied here: np.asarray on a box does not fail, it drops the gradient. Weibull is annotated against that vocabulary and added to the enforced list -- 18 functions, of which sf/ff/df/hf/Hf are one repeated shape the other 24 distributions share. Annotating a subclass does not require annotating ParametricFitter first. The base declares log_df(self, x, *params) where Weibull declares log_df(self, x, alpha, beta), which would be an override violation if the base were typed; it is not, so mypy treats it as Any and skips the check. The distributions can therefore be done one at a time, and the undeclared Hf/hf contract on the base class stays a separate problem rather than a blocking one. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
The changelog said it "does not fail; it drops the gradient". Both
halves were imprecise. It wraps the box in a 0-d object-dtype array,
which still computes the right value, because object arrays dispatch
arithmetic back to the box. Only the derivative is affected, and how
depends on the arithmetic:
alpha * x + beta plain [6. 3.] asarray [0. 3.]
(x / alpha) ** beta plain [-1.613 -0.094] asarray TypeError
The power form raises, because its backward pass needs a log of the
converted value and the box has no callable log. The product does not
raise at all -- it returns a zero gradient, which an optimiser reads as
"this parameter does not affect the likelihood", so the fit leaves the
parameter at its initial guess and reports success. That silent case is
the one the note is there to warn about, so it now says so.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
The note said a box is not a float or an ndarray, but not that mypy cannot check the claim either way -- which is the part that catches people, because both obvious narrowings look reasonable and neither is detectable by running the type checker. npt.ArrayLike gives 25 errors on `(x / alpha) ** beta`, and the fix that clears them is np.asarray, which is correct in the non-parametric packages and destroys the gradient here. npt.NDArray | float gives no errors and is simply false; py.typed then publishes it. Also records that the runtime types came from instrumenting a fit, since that is the only place they are visible. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
Boxable was Any, which is not the type -- it was a concession to
autograd shipping no type information, so mypy saw ArrayBox as a value
and rejected it in a union with "Variable is not valid as a type". The
concession cost the checking the ratchet exists to provide: under Any,
`Weibull.Hf(1.0, "not a number", 4.0)` was accepted in silence.
stubs/ describes the one autograd type that appears in surpyval's own
signatures, and Boxable is now npt.NDArray | float | ArrayBox, which is
what a fit actually passes. The bad call above is an error again.
Supplying any stub for a package makes mypy consider the whole package
described, so ignore_missing_imports stopped covering autograd and 328
unrelated attribute errors appeared. The __getattr__ stubs at
autograd/__init__.pyi and autograd/numpy/__init__.pyi keep the rest of
the library exactly as untyped as it was.
Applying this to Weibull exposed a second thing. `Weibull:
ParametricFitter` erased the concrete type, and the base declares none
of sf, ff, df, hf, Hf, qf or mean -- so with py.typed shipped, the
example in sf's own docstring did not type check for a user:
Weibull.sf(x, 3, 4)
error: "ParametricFitter" has no attribute "sf"
It is now annotated Weibull_. The annotation cannot just be dropped and
inferred: the regression subpackages and fit_best import the name, and
without an explicit type mypy cannot resolve it through that cycle. The
other 16 distributions annotated ParametricFitter have the same
erasure and are fixed as each is done.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
102 signatures across Bernoulli, BetaGeometric, Binomial,
DiscreteWeibull, Geometric, NegativeBinomial, Poisson and the
Discretize wrapper, all eight added to the enforced list. 83 followed
one rule -- for sf/ff/df/hf/Hf/qf and the log forms the first argument
after self is the evaluation point and the rest are parameters, with cs
taking two -- and 19 did not.
Checking the bodies found four things.
_parameter_initialiser was annotated wrong twice over, copied from
Weibull: these five return np.array(...) rather than a tuple, and x is
indexed with a boolean mask so it cannot be the scalar-or-array
Numeric. The base's implementations genuinely disagree on the return
shape; callers coerce either, so nothing is broken, but the signatures
now say which is which.
Discretize cannot hold the distribution it wraps as a
ParametricFitter, because the base declares none of sf, ff, df, hf, Hf,
qf or _parameter_initialiser -- nine attr-defined errors. The parameter
stays annotated, since that is the contract for callers; the attribute
is Any with the reason recorded.
Bernoulli.fit and Binomial.fit do not honour ParametricFitter.fit, and
the divergence is real rather than a typing artefact:
Bernoulli.fit([0,1], c=[0,0]) TypeError
Bernoulli.fit([0,1], how="MLE") TypeError
Binomial.fit([1,2], n_trials=3, how=..) TypeError
Both have closed-form MLEs and support neither censoring nor an
alternative method, so generic code written against ParametricFitter
fails on them. Recorded at each site rather than changed here.
The np.atleast_1d rebind pattern hides the array type, because
surpyval.np is autograd's numpy and therefore Any, and assigning Any
back to a declared parameter does not narrow it. Distinct locals
instead. Numeric output was diffed against the pre-change tree, over
list and array inputs, and is identical.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
ParametricFitter.fit takes 18 named arguments and three distributions
cannot honour any of them. Bernoulli, Binomial and ExactEventTime
estimate their parameters in closed form and accept only x and at most
c, n and t, so they overrode fit with a narrower signature. That is a
real divergence, not a typing nicety:
Bernoulli.fit([0, 1], c=[0, 0]) TypeError
Bernoulli.fit([0, 1], how="MLE") TypeError
Binomial.fit([1, 2], n_trials=3, how="MLE") TypeError
Code written against a ParametricFitter broke on exactly those three,
and nothing said so until it ran. An audit of all 25 subclasses found
these three and no others; the other 22 inherit fit unchanged.
fit and the twelve methods it needs now live on OptimisedFitMixin,
which the 21 distributions that have them inherit alongside
ParametricFitter. Every distribution is still a ParametricFitter --
that is what the isinstance gates in parametric, mixture_model,
parametric_regression_model, frailty_model and renewal_model check, and
what carries the distribution functions, the likelihood, _moment,
_set_support and from_params.
The point is that the wrong thing is now unwriteable rather than merely
undocumented. fit_best's candidate list is typed list[OptimisedFitMixin],
so adding one of the three is a type error rather than a runtime one.
Both `# type: ignore[override]` markers on fit are gone as a result,
which is the test of whether the split fixed the problem or hid it.
Typing that list forced the second half. The 16 remaining exports still
read `Normal: ParametricFitter = Normal_("Normal")`, which erases the
concrete class -- and since the base declares none of sf, ff, df, hf,
Hf, qf or mean, the example in each distribution's own docstring did
not type check for anyone whose checker honours py.typed. They name
their concrete class now.
from_params is untouched by this, because every distribution has one.
Bernoulli and ExactEventTime renamed the base's `params` argument to
`p` and `T`, so positional calls work and keyword calls raise, and
Bernoulli's `p` collides with the base's `p` (the limited-failure
proportion) to mean something unrelated. That needs a rename with a
deprecation alias and is left for its own change.
No behaviour changed. The split was done by line-range extraction with
an assertion that the spans tile the class body exactly, and the result
was diffed against the pre-change tree over 73 values -- every
distribution, four estimation methods, aic, neg_ll, mean, fit_best and
a mixture fit -- with no differences.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts
BREAKING: Bernoulli.from_params and ExactEventTime.from_params renamed
their first argument to `params`. It was `p` and `T` while the base
calls it `params`, so positional calls worked and keyword calls raised:
Bernoulli.from_params(0.5) OK
Bernoulli.from_params(params=0.5) TypeError
That is the shape of bug a test suite never catches, because every
internal call and every docstring example passes positionally. It only
breaks for someone writing generic code, on two of twenty-five
distributions.
Bernoulli's was worse than a rename. The base's `p` is the proportion
that never fails, so `p=0.5` meant the never-fails fraction on
twenty-four distributions and the event probability on Bernoulli --
same keyword, sibling classes, unrelated meanings, no error either way.
Bernoulli.from_params(p=...) and ExactEventTime.from_params(T=...) now
raise TypeError. Nothing changes meaning silently: `params` has no
default, so the old keyword forms fail loudly rather than being
reinterpreted as the never-fails proportion. There are no keyword
callers in the repository or the documentation.
All three also take gamma, p and f0 now and reject them with a
ValueError naming the distribution, via reject_structural_params.
Accepting-and-rejecting rather than omitting is what makes the
signatures match the base, so these can be called through a
ParametricFitter reference at all -- and it removes the last two
`# type: ignore[override]` markers. Every marker introduced by this
work is now gone; none were left as permanent suppressions.
Thirteen tests pin the contract: keyword and positional agreement,
rejection across all three distributions and all three structural
arguments, and one asserting each signature is a superset of the
base's, which is the property that made this a bug rather than a
naming preference.
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
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.
27 commits, 77 files, +2538/−591. It began as the
docsextra from #141 and grew each time validating one thing exposed another: the extra revealed a broken docs build, the docs fix silently broke lint, the broken lint hid mypy for ten commits, and turning mypy back on found real bugs in the distributions.1.
docsextra (part of #141)pip install -e ".[docs]"installs the documentation toolchain.docs/requirements.txtis removed rather than kept beside it — two copies of the same pinned toolchain is the arrangement that drifts, and the repo already settled this pattern for tests. Read the Docs installs the extra viaextra_requirements.Pins carried over unchanged, including
ipykernel==6.31.0and the reason for it.2. The docs build was broken on
developValidating the extra meant running a real build, which failed:
A
jupyter-executecell looped over['MPP', 'MOM', 'MSE', 'MPS', 'MLE']for a shifted Gamma; #338 madeGamma.fit(how="MPP")raise. Documentation cells execute during the build, so the build failed. The surrounding prose had gone stale the same way and now explains why the Gamma has no probability plot at all.3. What CI runs, where
Nothing caught the above, because CI never built the documentation and Read the Docs builds only
master. Separately, the full suite on everydevelopPR was the slowest part of working on the package.developmaster(release)master/ tagBoth conditions are now proven in both directions — PR #340 was opened into
masterpurely to confirm the jobs fire, and they did. (The original version of this description carried a caveat that only the skip half was proven; that caveat is now resolved.)scripts/check_all_pythons.pyis the other half of the trade: one command runs the suite and both doctest passes on 3.11/3.12/3.13 and refuses to say "passed" unless all nine did.Contributing.rstcarries the table and the timings.4. Documentation: API pages, then
-WNew reference pages for the previously undocumented public surfaces (multivariate, beta, frailty, Buckley-James; extended degradation). All 18 build warnings cleared — mostly
autosectionlabelduplicates against the changelog's repeated headings, fixed withautosectionlabel_maxdepth = 1rather than by suppression.-Wis now on in both CI and.readthedocs.yaml; enabling one without the other lets a build go green while publishing broken pages.5. #287 — the survival tree's log-rank split statistic was wrong
kind="non-parametric"trees, and anyRandomSurvivalForestbuilt from them, selected splits on a statistic off by factors of several — and reordered, not merely inflated: of two candidate splits, 1.8564 → 0.2765 and 0.2765 → 1.2247. The at-risk count is now computed asY(τ) = #{x ≥ τ} − #{tl ≥ τ}via two suffix sums and asearchsorted, which is also O(N log N) rather than the previous O(N×G). Verified against a naive reference over 200 random partitions plus left-truncation, censored-tail and tie cases.6.⚠️ BREAKING:
from_paramsargument namesParametricFitter.from_paramsnames its first argumentparams.Bernoullicalled itpandExactEventTimecalled itT, so positional calls worked and keyword calls raised:That is the shape of bug a test suite never catches, because every internal call and every docstring example passes positionally. It only broke for someone writing generic code, on two of twenty-five distributions.
Bernoulli's was worse than a rename. The base's
pis the proportion that never fails. Sop=0.5meant the never-fails fraction on twenty-four distributions and the event probability on Bernoulli — the same keyword, sibling classes, unrelated meanings, and no error either way.What breaks:
Bernoulli.from_params(p=...)andExactEventTime.from_params(T=...)now raiseTypeError. Positional calls are unaffected and unchanged in value. Nothing changes meaning silently —paramshas no default, so the old keyword forms fail loudly rather than being reinterpreted as the never-fails proportion. There are no keyword callers in the repository or the documentation.All three closed-form distributions also accept
gamma,pandf0now and reject them with aValueErrornaming the distribution.7.
OptimisedFitMixin— the estimation machinery left the base classParametricFitter.fittakes 18 named arguments. Three distributions cannot honour any of them:An audit of all 25 subclasses found these three and no others.
fitand the twelve methods it needs moved toOptimisedFitMixin, which the 21 distributions that have them inherit alongsideParametricFitter.Every distribution is still a
ParametricFitter— that is what theisinstancegates inparametric,mixture_model,parametric_regression_model,frailty_modelandrenewal_modelcheck. The mixin only adds the estimation methods on top.The point is that the wrong thing is now unwriteable rather than merely undocumented:
Annotate a parameter
OptimisedFitMixinwhen it must be fittable by a chosen method,ParametricFitterwhen only the distribution functions are needed.8. Every distribution exports its own type
17 distributions read
Weibull: ParametricFitter = Weibull_("Weibull"), which erases the concrete class. Since the base declares none ofsf/ff/df/hf/Hf/qf/mean, the example in each distribution's own docstring did not type check for anyone whose checker honourspy.typed:The annotation cannot simply be dropped and inferred — the regression subpackages and
fit_bestimport these names, and mypy cannot resolve them through that cycle. They name the concrete class instead.9. Type-hint ratchet (#143) — 17 modules enforced
disallow_untyped_defsis set per-module, so an unannotated function in a listed module is an error and coverage cannot slip back. A module joins once it is clean; the remaining 1206 unannotated functions do not have to be finished first.Enforced:
distribution,serialisation,metrics.*,univariate.information_criteria,datasets, all ofunivariate.nonparametric.*,recurrent.nonparametric.*,univariate.regression.frailty.*, and 9 of the 25 parametric distributions (Weibull plus all 8 discrete).Annotating is what makes mypy check a function body, and that found real defects rather than paperwork:
SerialisableMixin.to_json/from_jsoncalledto_dict/from_dict, which the mixin never declared. Now declared underTYPE_CHECKING— a real stub would be inherited, andcopula_modelprobes withhasattr.ESTIMATOR_FUNCSwas built fromnonp.nelson_aalenand siblings while the package__init__was still running, so the names resolved to the submodules, not the functions. It worked by load order, not by guarantee.success_runtestedconfidenceandalphafor truthiness, so passing both with either set to zero skipped the "only one of" raise, andconfidence=0fell through every branch leavingalphaasNone.turnbull,rank_adjustandNonParametricCounting.from_xrddeclared array-like parameters and then indexed and divided them._logrank_z_vdeclaredc/nas arrays while handlingNonefor both.NonParametricCounting.fitdeclaredwindowsas array-like when it is the dictionary its own docstring describes._parameter_initialiserdisagrees on its return type across distributions — Weibull returns a tuple, the discrete ones return arrays. Harmless today; the signatures now say which is which.The distributions needed a type vocabulary first
A distribution deals in two kinds of value and only one can be an autograd box. Instrumenting a live fit showed the runtime types directly:
So
Numeric = npt.NDArray | floatfor what a function is evaluated at, andBoxable = npt.NDArray | float | ArrayBoxfor a parameter or anything derived from one.ArrayBoxneeded a stub, because autograd ships no type information and mypy otherwise rejects it in a union.stubs/autograd/describes that one class; permissive__getattr__stubs at the package level keep the rest of autograd exactly as untyped as it was (supplying any stub makes mypy consider the whole package described, which otherwise produced 328 spurious errors).This matters beyond tidiness. The
np.asarrayfix applied toturnbullandmcfabove would be a silent numerical bug here.np.asarrayon a box does not reject it — it wraps it in a 0-d object array, the forward value stays correct, and only the derivative is damaged:A zero gradient is not an error to an optimiser; it means "this parameter does not affect the likelihood". The fit leaves the parameter at its initial guess and reports success.
10. The lint job had been red for ten commits
Fix the last two documentation build warningsturned a plain reference into a:class:cross-reference and pushed one line to 80 characters. flake8 runs before mypy in the lint job, so from that commit on mypy was skipped, not run — including through three ratchet batches. Bisecting:All fixed. A related finding: mypy's result is interpreter-dependent —
d_ghosts, an array in one branch and a scalar in the other, is rejected on 3.12 and accepted on 3.11. A local mypy run on one interpreter is not a proxy for the lint job.Verification
scripts/check_all_pythons.py: 9/9 green on 3.11/3.12/3.13 — suite + both doctest passes — re-run after every batch. Final:2166 passed, 271 skipped, 5 xfailed,229doctests each.sphinx -b html -W --keep-goingexits 0.OptimisedFitMixinsplit (every distribution, four estimation methods,aic/neg_ll/mean,fit_best, a mixture fit) and a separate diff for the discrete batch — no differences in either.# type: ignore[override]introduced during this work has been removed by fixing the underlying divergence. None remain as suppressions.Known-imperfect, stated rather than glossed
mixture_model.py) were swept in bygit add -A; the changes are oneisortimport-reordering line each, zero behaviour, and were covered by the full test run. The commit messages do not mention them.isortnon-conformance is repo-wide and pre-existing — 28 files at the branch point, 15 now. It runs in pre-commit, not CI.Follow-ups, not included
: ParametricFittererasure (§8 fixed 16 of 31; the rest go as each file is annotated).check_all_pythons.py— its docstring reasons that "lint runs on every pull request anyway", which §10 disproves twice over.sf/ff/df/hf/Hf/qfonParametricFitter. Its docstring states the contract; the class does not express it, which is whyDiscretizeholds its wrapped distribution asAny.developto close the detection window from a release cycle to a day.🤖 Generated with Claude Code
https://claude.ai/code/session_01TRhKL2fJBiAAfNo9rihwts