Skip to content

Make the signal-ladder renames backward compatible, and ship 1.77.0 instead of 2.0.0 - #535

Merged
kgdunn merged 2 commits into
mainfrom
claude/critical-review-planning-29pkvp
Aug 30, 2026
Merged

Make the signal-ladder renames backward compatible, and ship 1.77.0 instead of 2.0.0#535
kgdunn merged 2 commits into
mainfrom
claude/critical-review-planning-29pkvp

Conversation

@kgdunn

@kgdunn kgdunn commented Aug 30, 2026

Copy link
Copy Markdown
Owner

Summary

  • Signal-ladder API renames, and two real defects the naming proposal missed (2.0.0) #534 landed the signal-ladder renames as a breaking 2.0.0, removing four public names with no deprecation cycle. Nothing had been published (no v2.0.0 tag; latest is v1.75.2), so this reworks the same change into the Announce phase docs/development/deprecation_policy.rst calls for: every old spelling still works and emits a DeprecationWarning naming its replacement, which makes the release purely additive and correctly a MINOR. Version is now 1.77.0, with CITATION.cff in step.
  • Restored as forwarding shims: permutation_q2, pipeline_null, discriminate_observational. The permutation_q2 shim keeps the old argument order and adapts, since check_predictive_signal now takes the blocks first. All three take explicit signatures rather than **kwargs, so help() and inspect.signature stay accurate.
  • Restored as deprecated output keys, alongside their replacements and carrying identical values: q_value (→ p_value_fwer), discriminator_significant (→ is_predictive), result.relate["discriminator"] (→ ["predictive_descriptors"], same object).
  • Restored the discriminator= keyword on relate_observational, analyze_descriptive and the sensory_analyze_descriptive tool input. Passing both spellings raises ValueError rather than silently picking one, following the desirability_weights precedent in experiments/optimization.py.
  • empirical_fdr is returned again, still clipped to [0, 1] exactly as before, so an existing caller reads the same number it always did. null_to_observed_ratio is the same quantity unclipped; the two only diverge once shuffling out-finds the real response, which is the case the old key was hiding.

Removal of all of the above is scheduled for 2.0.0.

The substance of the rename is unchanged from #534, including the two defects it fixed: the find_predictive_descriptors docstring claimed Benjamini-Hochberg correction while the code builds a Westfall-Young max-statistic null, and the field named q_value held a family-wise-error-adjusted p-value while q_value in the sibling associations list of the same dict genuinely is a BH q-value.

Test plan

  • Full uv run pytest suite with the coverage gate: 2984 passed, 3 skipped, coverage 94.46% against the 92% gate
  • uv run pytest tests/test_sensory.py tests/test_sensory_end_to_end.py green: 63 passed
  • uv run pytest tests/test_multivariate_null.py green, including the new TestDeprecatedAliases
  • New coverage: each old function name warns and returns a result identical to its replacement; the deprecated descriptor fields mirror their replacements exactly; the discriminator= keyword warns and both result keys reference the same object; passing both keyword spellings raises; empirical_fdr is still clipped and equals min(null_to_observed_ratio, 1.0)
  • The package does not warn on itself: the default relate_observational path runs clean under -W error::DeprecationWarning
  • uv run ruff check . and uv run ruff format --check . both pass
  • uv run mypy src/process_improve passes, 153 files

The one CI failure on this head was test (3.13, macos-latest) hitting the known pulp/CBC-under-Rosetta flake in tests/test_omars_ilp.py, which this PR does not touch. It passed on re-run of the same commit, confirming the intermittency. Details in the comment below.

Checklist

  • Version bumped in pyproject.toml to 1.77.0 (MINOR: additive only, nothing removed), with CITATION.cff set to the identical version in the same commit
  • Tests added or updated where relevant
  • ruff check . passes
  • CHANGELOG.md updated, with an ### Added and a ### Deprecated section replacing the previous ### Removed

claude added 2 commits August 30, 2026 13:29
The renames landed in #534 as a breaking 2.0.0. Nothing had been published, so
this reworks them into the Announce phase the deprecation policy calls for:
every old spelling still works and warns, and the release is a MINOR.

Restored, each forwarding to its replacement with a DeprecationWarning naming
it, per docs/development/deprecation_policy.rst:

- permutation_q2, pipeline_null, discriminate_observational as function shims.
  The permutation_q2 shim keeps the old argument order and adapts to the new
  signature, since check_predictive_signal now takes the blocks first.
- q_value and discriminator_significant on the descriptor records, emitted
  alongside p_value_fwer and is_predictive with identical values.
- result.relate["discriminator"], referencing the same object as
  result.relate["predictive_descriptors"].
- The discriminator= keyword on relate_observational, analyze_descriptive and
  the sensory_analyze_descriptive tool input. Passing both spellings raises
  ValueError rather than silently picking one, following the
  desirability_weights precedent in experiments/optimization.py.
- empirical_fdr, still clipped to [0, 1] exactly as before, so an existing
  caller reads the same number it always did. null_to_observed_ratio is the
  same quantity unclipped; the two only diverge once shuffling out-finds the
  real response, which is the case the old key was hiding.

The shims take explicit signatures rather than **kwargs so help() and
inspect.signature stay accurate, matching the reasoning behind _model_method
elsewhere in the package.

Verified that the package does not warn on itself: the default
relate_observational path runs clean under -W error::DeprecationWarning.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016vCXGaB7zcXSd7GXQoHJig
test_keys_are_as_documented asserts the exact returned key set, and bringing
empirical_fdr back as a deprecated alias made it fail. Caught by CI on
3.13/ubuntu, not locally: after adding the key I re-ran only the -k Deprecated
subset of the file rather than the whole file, so the one test that pins the
full set never ran.

The key is listed with a note saying it is deprecated and due out in 2.0.0, so
removing it then fails this test rather than passing silently.

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

kgdunn commented Aug 30, 2026

Copy link
Copy Markdown
Owner Author

test (3.13, macos-latest) is red on 33f7f8a, and it is not this PR's.

The failure: tests/test_omars_ilp.py::test_multistart_is_deterministic_for_seedpulp.apis.core.PulpSolverError executing pulp's bundled CBC binary at solverdir/cbc/osx/i64/cbc.

Why it is not this PR's:

  • This PR touches no file under src/process_improve/experiments/ and does not touch tests/test_omars_ilp.py. Its diff is confined to the multivariate and sensory null/rename work plus version metadata.
  • The repository already documents this exact failure mode. tests/test_omars_ilp.py:272 carries a skipif(sys.platform == "darwin") on a sibling test with the reason: "pulp's bundled CBC (an Intel binary run under Rosetta on Apple Silicon CI runners) intermittently exits nonzero under this test's 40+ rapid solver spawns." The failing test asserts report.n_restarts == 40, so it drives the same solver-spawn storm.
  • It is intermittent, not a regression. On the previous head of this same PR (f3d0f6b) the macOS job reported 1 failed, 2971 passed, 15 skipped, and the single failure there was test_keys_are_as_documented — so this OMARS test passed on macOS one commit earlier. The only change between the two commits is one expected key set inside tests/test_multivariate_null.py, which cannot affect a CBC subprocess.

No fix ported, because none exists to port. The repo's own remedy for the sibling test is a macOS skip, and adding a skip to get a red job green is not something I will do on my own initiative — that is the maintainer's call, and it would be quarantining a test rather than fixing it. If you would like the same skipif extended to test_multistart_is_deterministic_for_seed, say so and I will push it; the reasoning at line 272 appears to apply verbatim.

Re-running the failed job once to confirm the intermittency. The other 3.13 legs (ubuntu, windows) and test-under-dash-O cover this property on platforms without the Rosetta issue.


Generated by Claude Code

@codecov

codecov Bot commented Aug 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@kgdunn
kgdunn merged commit 88c4af5 into main Aug 30, 2026
22 of 23 checks passed
@kgdunn
kgdunn deleted the claude/critical-review-planning-29pkvp branch August 30, 2026 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants