Repository navigation
Conversation
Overall assessment✅ Looks good — no unmitigated P0 or P1 findings. Executive summary
Methodology
Code QualityNo findings. Clone, protocol, and sample-weight validation are centralized and errors remain targeted. PerformanceNo findings. The additional configuration-time copies are bounded and appropriate for validating learner isolation. MaintainabilityNo findings. The same helpers cover DMLDiD and direct Tech DebtNo new untracked correctness debt identified. SecurityNo findings. Foreign copy-exception messages and traceback context are suppressed at Documentation/Tests
Test coverage is strong across panel/RCS, survey weighting, protocol loss, sanitized errors, and failed-refit cleanup. Runtime tests could not be executed because pytest/NumPy are unavailable; changed Python files passed AST parsing, the notebook passed JSON parsing, and the changelog-fragment checker passed. |
Context: - Cross-fit reuse after a failed or identity-returning deepcopy can carry fitted state and prior-fold training data into a later fold. - DMLDiD must reject an invalid learner template before any group-time cell is fitted. Changes: - Require a distinct top-level deepcopy for cross-fit learner templates and raise a sanitised TypeError otherwise. - Probe custom DML learner specifications during configuration validation while retaining the direct cross_fit_predict backstop. - Document the top-level clone boundary and cover copy failures, identity copies, sklearn templates, exact OOF isolation, and pre-cell error propagation. Verification: - pytest -q tests/test_crossfit.py tests/test_dml_did.py tests/test_changelog_fragments.py tests/test_docs_ia.py tests/test_doc_deps_integrity.py - Python 3.9 targeted regression tests, including sklearn preflight - ruff, black, scoped mypy, changelog compiler, and native cross-fit output comparison against upstream/main - Not run: full default suite was stopped after an interrupted long-running attempt.
Context: - Cross-fit learner templates must fail closed when cloning cannot provide an independent, usable learner, without exposing stale DML results after a failed re-fit. Changes: - Validate clone protocol and weighted-fit capability during DML preflight and for each direct cross-fit fold; delayed errors name the public nuisance parameter. - Clear DMLDiD fitted state before every fit attempt, and cover panel, repeated-cross-section, survey, and direct helper failure paths. - Correct the DML tutorial and documentation registry, and record the warning-to-TypeError behavioural change. Verification: - /tmp/diff-diff-814-python39/bin/python -m pytest -q tests/test_crossfit.py tests/test_dml_did.py tests/test_survey_dml.py tests/test_changelog_fragments.py tests/test_docs_ia.py tests/test_doc_deps_integrity.py (665 passed, 6 skipped) - black --check, ruff check, and mypy --follow-imports=skip on touched source/test modules - python -m json.tool docs/tutorials/32_dml_did.ipynb and python .claude/scripts/changelog_compile.py check - Not run: full repository test suite.
Context: - PR #816 review identified overbroad pre-cell failure claims and optional sklearn tests that CI never exercised. Changes: - Adapt the documentation corrections from mirror commit 50109fe to distinguish preflight failures from later hard TypeErrors. - Require unfitted templates and distinct copies in both seed descriptions. - Add unpinned scikit-learn to the dev extra and both Rust workflow dependency lists; retain importorskip guards. - Leave estimator execution and the separate StackedDiD sentinel contract unchanged. Verification: - Python 3.12 focused cross-fit, DML, changelog and documentation suites: 582 passed. - Both sklearn integration tests passed without skips on Python 3.9. - Black, Ruff, changelog compiler, TOML/YAML and notebook JSON validation passed. - Known failure: test_sklearn_clone_round_trip_if_available[StackedDiD] fails on sentinel parameter equality with sklearn installed; reproduced on Python 3.9 in both this branch and unchanged base e982340. - Not run: full repository suite, Sphinx build or remote CI matrix.
Context: - Adding scikit-learn to CI enables the StackedDiD clone round-trip test, which fails because copied omitted-parameter sentinels compare unequal. Changes: - Return self from the stateless _NotSupplied marker's shallow and deep copy methods. - Cover sentinel identity, isolation of neighbouring mutable values, and canonical and deprecated StackedDiD control parameters during sklearn cloning. - Preserve alias resolution, deprecation warnings and existing pickle behaviour; add a changelog fragment and correct the clone-test dependency description. Verification: - Reproduced six copy/clone failures before the source fix. - Python 3.13 focused estimator, clone, changelog and documentation checks: 1010 passed, 13 skipped. - Python 3.9 estimator, clone, survey DML, methodology and bad-control parity checks: 790 passed, 13 skipped, 5 deselected. - Ruff and Black across diff_diff and tests, whole-package mypy, changelog compiler, notebook JSON and git diff --check passed. - Not run: Rust backend tests, notebook execution and remote CI matrix. The broader pure-Python suite is still running.
Context: - The rebase retained two descriptions of the removed warning-and-reuse path, contradicting the fail-closed learner contract. Changes: - Align the methodology registry and documentation dependency note with lazy per-fold cloning and sanitised TypeError propagation. - State that failures never reuse the template or become DegenerateFoldError skips; leave runtime code and unrelated baseline failures unchanged. Verification: - Documentation-dependency, documentation-IA, tutorial-drift and changelog tests: 367 passed. - Changelog compiler, YAML parsing, stale-fallback text scan and git diff --check passed. - Not run for this documentation-only correction: full suite, Rust matrix, Sphinx build or notebook execution; Sphinx and nbmake are absent from the verification environment.
igerber
force-pushed
the
mirror/816-dml-crossfit-isolation
branch
from
September 27, 2026 12:09
50109fe to
ac1ef63
Compare
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment✅ Looks good — no unmitigated P0 or P1 findings. Executive summary
Methodology
Code QualityNo findings. Clone, protocol, and weighted-fit validation are centralized in PerformanceNo findings. The configuration-time copies add bounded overhead. Maintainability
Tech DebtNo new untracked correctness debt. SecurityNo findings. Copy-exception text and traceback context are suppressed, while only exception and learner class names are exposed. Documentation/TestsNo findings. Both behavioral changes have changelog fragments, and the prior documentation ambiguity is fixed. Static parsing and the changelog-fragment check passed. Runtime tests could not run because the environment lacks pytest and the scientific Python dependencies. |
…optional at runtime Supersedes the 2026-08-01 decision row that kept scikit-learn out of the dev extra; #816 adds it (unpinned) to the dev extra and both rust-test.yml install steps so the sklearn clone round-trip and DML cross-fit learner tests always run in CI. Runtime posture unchanged; importorskip guards stay.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment✅ Looks good — no unmitigated P0 or P1 findings. Executive summary
Methodology
Code QualityNo findings. Clone, protocol, and weighted-fit validation are centralized in PerformanceNo findings. Preflight copies add bounded configuration-time overhead. MaintainabilityNo findings. The prior P2 is resolved: Tech DebtNo new untracked correctness debt. SecurityNo findings. Copy-exception messages and chaining are suppressed while retaining actionable class names. Documentation/TestsNo findings. Both user-visible changes have valid changelog fragments, and tests cover panel/RCS propagation, bad-control stages, survey weighting, stale-state clearing, sanitization, and sentinel cloning. In-memory compilation and the changelog-fragment check passed. Runtime tests could not run because this environment lacks pytest and NumPy. |
Owner
Author
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
3 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.
Mirror of third-party PR #816 (fixes #814) so the label-gated CI and the CI AI reviewer can run on the contributor's change. The contributor's two commits are carried verbatim; one maintainer commit on top narrows a documentation claim. Do not merge this PR on its own - findings feed back to #816.
Summary
deepcopyfails or returns the original object at configuration time, before any group-time cell is fitted (contributor commit 9cd5649).cross_fit_predictcallers; a per-fold clone failure propagates as a sanitizedTypeError, never a NaN-cell skip; a failed re-fit clears prior fitted state; fold clones are re-validated for the learner protocol and, on declared survey fits,sample_weightsupport (contributor commit bb187a3).Methodology references (required if estimator / math changes)
_crossfit.py,_learners.py,dml_did.py)Validation
tests/test_crossfit.py,tests/test_dml_did.py(copy failure, self-returning__deepcopy__, clone protocol loss, weighted clone loss, panel/RCS hard-error propagation, re-fit state clearing, survey preflight, sanitized traceback, sklearn preflight viaimportorskip)Security / privacy
token=SECRET-...strings in the new tests are fake sentinels asserting that foreign exception text is withheld)Changelog
changelog.d/20260903-dml-crossfit-learner-isolation.md)