[imerad]

Description

Moves the validation and computation of IPWClassicalMeanEstimator into a new internal IPWClassicalMeanEngine, so estimate becomes a thin shell calling the engine once, like the other migrated estimators. The engine can now be reused by an IPW-based monitor and by ASIMeanEstimator.

As a side effect, estimate now raises a ValueError when y and sampling_probability differ in length or when y has fewer than 2 labeled values (previously an opaque indexing error or a NaN standard error).

Closes #506

Checklist

Quality gates that must be satisfied before requesting a review:

• I have read CONTRIBUTING.md
• make lint passes
• make type-check passes
• make tests passes
• make coverage reports 100% coverage
• make test-notebooks passes
• New public API has numpy-style docstrings
• New public API is inserted in the API reference section of the documentation
• Docs build without warnings (make doc)
• CHANGELOG.md updated if the change is user-facing

LLM usage

Disclose if an LLM was used in writing this PR:

• No LLM used
• I used an LLM and I went through and validated all the code myself

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[imerad]

Code review

Six findings, most severe first.

1. Behaviour change in a refactor: new _validate_min_samples check

glide/engines/ipw_classical.py:24

_validate_min_samples(y[labeled_mask], "y") makes IPWClassicalMeanEstimator.estimate reject inputs it previously accepted. Example: y=[5.0, nan, nan] with sampling_probability=[0.5, 0.5, 0.5] has one label. Before this PR, estimate returned the IPW mean, with std from ddof=1 over all n_samples rows (the zero-filled unlabeled rows included). Now it raises ValueError("'y' must have at least 2 non-NaN values; got 1.").

Under IPW the variance is computed over the weighted terms of all rows, so the relevant requirement is n_samples >= 2, not the labeled count. The check looks copied from ClassicalMeanEngine, where it is correct. Callers with very small label budgets now crash. The Raises docstring changed, but CHANGELOG.md was not updated for this user-visible change.

Suggested fix: drop the check, or keep it deliberately and document it in CHANGELOG.md.

2. Lost test coverage of zero-probability row filtering

tests/unit/engines/test_ipw_classical.py:28

test_preprocess_valid_output uses all-0.5 probabilities and no zero-probability row, so the row-filtering branch is never asserted. CLAUDE.md says to put edge-case rows (a zero-probability row, a NaN label) into the single valid-output example. The delegation test mocks _get_non_zero_mask to all-True, so nothing else covers it. The old estimator test did (sampling_probability=[0.5, 0.5, 0.0, 0.5]). If non_zero_mask were dropped or inverted, the test would still pass.

3. Unused labeled_indicator in the dataset

glide/engines/ipw_classical.py:28

_compute_mean_and_std never uses labeled_indicator. Its only consumer is the estimator, which unpacks _, labeled_indicator, _ and calls int(labeled_indicator.sum()) to get n_labeled. The .astype(float) is unnecessary, since a boolean array sums fine. The 3-tuple dataset is harder to read than the plain array of the sibling ClassicalMeanEngine. A simpler design would be a 2-tuple dataset, or a boolean mask.

4. Estimator tests are weak guards

tests/unit/estimators/test_ipw_classical.py:33

test_estimate_delegates builds the expected dataset by calling estimator._engine._preprocess outside the mock block, so the wraps= mocks compare outputs of the code under test with each other. CLAUDE.md asks for hardcoded expected values. If _preprocess returned a wrong dataset, the test would still pass. test_init_sets_engine only asserts isinstance and adds little over the estimate tests.

5. Dead plumbing: no-op _fit_tuning_parameter call (minor)

glide/estimators/ipw_classical.py:77

estimate calls _fit_tuning_parameter(dataset, power_tuning=False), which always returns None, and passes that None to _compute_mean_and_std. ClassicalMeanEstimator skips the tuning step and passes None directly, so the two estimators are inconsistent. Harmless, but the extra call only exists to satisfy the protocol.

6. _preprocess signature differs from the protocol (minor, low severity)

glide/engines/ipw_classical.py:14

_preprocess(self, y, sampling_probability) takes positional parameters, while MeanEstimationEngine in base.py declares _preprocess(self, **fields). This could trip ty or generic keyword-based callers (e.g. a monitor façade). The existing PPI and classical engines have the same shape, so this is probably acceptable.