Share one spectrum across a centroid's fits, in both optimisers - #46
Merged
Merged
Conversation
`_spectrum_key` was a second instance attribute that had to be kept in step with `last_spectrum` at every exit, and it only ever knew the provenance of the single most recent spectrum. Reuse survived only because `asarray` returns an already-float64 array unchanged, so the arrays could be matched by identity -- correct, but three lines of comment defending a coincidence. `_Spectrum` is a tuple subclass carrying its own `provenance`, so the provenance travels with the object instead of beside it. That removes `_spectrum_key`, the `ours` flag and the identity `zip`, and it is strictly more capable: a spectrum held across an intervening call no longer loses its label. It unpacks, indexes and pickles as a plain triple, so `spectrum=`, `last_spectrum` and Global_CPD's `fit_spectrum(*...)` are unchanged, and a triple built anywhere else still has no provenance and is taken as given. `taper` and `process_subgrid` are dropped from what is checked. Holding a callable on the instance kept its captured scope alive, and worse, made `pickle.dumps(grid.optimise)` raise for a closure or a lambda -- so one `optimise(..., process_subgrid=...)` sent `parallelise_routine` to serial for every later call on that instance, warning that it could not send `func`, which was perfectly picklable. Measured before this change: 48 centroids on 4 processors went 0.16 s -> 0.48 s and stayed there. The misuse worth catching is a spectrum from a different window or centroid, which `window`, `xc`, `yc` and `dof_factor` still catch. `parallelise_routine` now rejects `spectrum` outright, alongside `on_error` and `seed`. One spectrum cannot describe a list of centroids: forwarded, it gave every centroid the same answer -- a flat map that looks like a result -- and since the provenance warning fires per call, whether the user was told depended on how many processors were available. Also, `residuals` claimed `np.errstate` is thread safe where `catch_warnings` is not. It only became context-local in NumPy 2.0 and `pyproject.toml` allows `numpy>=1.20`, so on the supported floor it sets global state through `seterr` just the same. The two arguments that do hold on every version -- it does not swallow unrelated warnings, and it is cheaper to enter several hundred times per fit -- are enough on their own. Tidying alongside: the four identical `spectrum :` parameter blocks collapse to one description on `optimise` and three cross-references, the pattern already used for `dof_factor`; the `optimise` example passes its fitted parameters into `profile`, which is worth 5% of the pair and stops the bound names going nowhere; and the new tests use `monkeypatch`, `pytest.warns` and `recwarn` in place of hand-rolled restores and warning capture. CLAUDE.md's test counts were left behind by the previous two commits and are now measured rather than incremented. Suite 121 -> 123 tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PLgEaDtuZw6QXDphSrzcdQ
`_resolve_spectrum`, `_Spectrum` and `last_spectrum` sat on `CurieOptimiseBouligand`, so `CurieOptimiseTanaka` recomputed a spectrum on every call. It has the stronger case of the two: two straight-line fits cost almost nothing beside computing the spectrum -- 24.8 ms of a 25.9 ms `optimise` at a 1025-cell window, 96% -- and the documented workflow is a band sweep, where `check_bands` needs `k`, then `optimise` needs all of it, then `sensitivity`, then a revised band needs it again. None of those depend on the bands. Measured on `optimise` / `check_bands` / `optimise`, 51.9 ms -> 26.6. The mechanism was already class-agnostic -- it never mentions `power`, which each `_spectrum` pins below the seam -- so it moves to `CurieGrid` beside `window_spectrum`. Two things were Bouligand-specific and both were parameters in disguise. A subclass defining `_spectrum` now also defines: - `_SPECTRUM_ARGS`, the names of its positional arguments in order, so the provenance is picked out by name rather than by position. Tanaka takes seven with `beta` inserted at position five, Bouligand six. - `_SPECTRUM_PROVENANCE`, the subset a reused spectrum is checked against. Tanaka's includes `beta`, which subtracts the fractal contribution and so changes the values of `Phi`: a spectrum computed at one beta is the wrong data for a fit at another, and is now a warning rather than a silent answer. - `_SPECTRUM_RETURNS`, which fixes the arity -- four arrays here against three -- and names them when it is wrong, which is worth having where the order of `(k, Phi, Phi_n, sigma)` is not obvious. A call site is unchanged apart from the name: it forwards exactly what it forwarded before, in `_spectrum`'s own order. `optimise` and `sensitivity` take `spectrum=`. `parallelise_routine` already rejects it and is inherited, so the Tanaka routines get that for free -- verified rather than assumed. The three names are a second list to keep in step with a signature, which is the drift this refactor exists to remove, so `test_grid.py` pins them against `inspect.signature` and asserts no callable reaches the provenance. Suite 123 -> 129 tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PLgEaDtuZw6QXDphSrzcdQ
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.
Cleanup pass over the two spectrum-sharing commits (
0107ecb,dee8caf), from/simplify, then the generalisation that review recommended.1. Provenance rides on the spectrum, not beside it
_spectrum_keywas a second instance attribute that had to be kept in step withlast_spectrumat every exit, and it only ever knew the provenance of the single most recent spectrum. Reuse survived only becauseasarrayreturns an already-float64 array unchanged, so the arrays could be matched by identity — correct, but three lines of comment defending a coincidence._Spectrumis a tuple subclass carrying its ownprovenance. That removes_spectrum_key, theoursflag and the identityzip, and it is strictly more capable: a spectrum held across an intervening call no longer loses its label. It unpacks, indexes and pickles as a plain tuple, sospectrum=,last_spectrumand Global_CPD'sfit_spectrum(*...)are unchanged.A
process_subgridno longer disables multiprocessing for goodThe old key stored the
taperandprocess_subgridcallables. That kept their captured scope alive, and madepickle.dumps(grid.optimise)raise for a closure or lambda — so oneoptimise(..., process_subgrid=...)sentparallelise_routineto serial for every later call on that instance, warning that it could not sendfunc, which was perfectly picklable. Measured before the change: 48 centroids on 4 processors, 0.16 s → 0.48 s, and it stayed there.parallelise_routinerejectsspectrumAlongside
on_errorandseed. One spectrum cannot describe a list of centroids: forwarded, it gave every centroid the same answer — a flat map that looks like a result — and since the warning fires per call, whether the user was told depended on how many processors were available.Corrections and tidying
residualsclaimednp.errstateis thread safe wherecatch_warningsis not. It only became context-local in NumPy 2.0 andpyproject.tomlallowsnumpy>=1.20. The two arguments that hold on every version are kept.spectrum :parameter blocks collapse to one description plus three cross-references — the pattern already used fordof_factor.monkeypatch,pytest.warnsandrecwarnin place of hand-rolled restores and warning capture.2. Tanaka gets the same sharing
The mechanism was already class-agnostic — it never mentions
power, which each_spectrumpins below the seam — so it moves toCurieGridbesidewindow_spectrum.Tanaka has the stronger case of the two. Two straight-line fits cost almost nothing beside computing the spectrum — 24.8 ms of a 25.9 ms
optimiseat a 1025-cell window, 96% — and the documented workflow is a band sweep:check_bandsneedsk, thenoptimiseneeds all of it, thensensitivity, then a revised band needs it again. None of those depend on the bands. Measured onoptimise/check_bands/optimise: 51.9 ms → 26.6.Two things were Bouligand-specific, both parameters in disguise. A subclass defining
_spectrumnow also defines:_SPECTRUM_ARGSbetaat position five, Bouligand six_SPECTRUM_PROVENANCEbeta, which subtracts the fractal contribution and so changesPhi— now a warning rather than a silent answer_SPECTRUM_RETURNS(k, Phi, Phi_n, sigma)is not obviousCall sites are unchanged apart from the name.
optimiseandsensitivitytakespectrum=;parallelise_routinealready rejects it and is inherited, so the Tanaka routines get that for free — verified rather than assumed.Testing
Suite 121 → 129 tests, all passing. New coverage includes: a grid that ran
optimisewith a lambdaprocess_subgridstill pickles; a routine refuses a shared spectrum; Tanaka's four-array spectrum round-trips and itsbetais provenance while its bands are not; andtest_grid.pypins the three class attributes againstinspect.signature, since a second list to keep in step with a signature is the drift this refactor exists to remove.🤖 Generated with Claude Code
https://claude.ai/code/session_01PLgEaDtuZw6QXDphSrzcdQ