Skip to content

docs(todo): drop two resolved Actionable rows (rust vcov determinism #653; HonestDiD B2b)#662

Merged
igerber merged 3 commits into
mainfrom
chore/todo-resolved-row-cleanup
Jul 9, 2026
Merged

docs(todo): drop two resolved Actionable rows (rust vcov determinism #653; HonestDiD B2b)#662
igerber merged 3 commits into
mainfrom
chore/todo-resolved-row-cleanup

Conversation

@igerber

@igerber igerber commented Jul 9, 2026

Copy link
Copy Markdown
Owner

Summary

Methodology references (required if estimator / math changes)

  • Method name(s): N/A — TODO bookkeeping only
  • Paper / source link(s): N/A
  • Any intentional deviations from the source (and why): None

Validation

  • Tests added/updated: None
  • Backtest / simulation / notebook evidence (if applicable): N/A

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

Generated with Claude Code

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown

Overall Assessment

Looks good — no unmitigated P0/P1 findings. The PR is TODO bookkeeping only and does not change estimator code, math, variance/SE logic, identification assumptions, or defaults.

Executive Summary

  • The removed HonestDiD Δ^SD optimal-FLCI row is already documented as resolved in the Methodology Registry and covered by parity tests.
  • The removed Rust clustered-vcov nondeterminism row appears resolved in code and regression-tested.
  • No methodology mismatch, missing assumption check, or inference/NaN issue was introduced.
  • One minor stale test comment still references the deleted TODO row.

Methodology

  • Severity: None
    Impact: No estimator or methodology behavior changed. The relevant HonestDiD Δ^SD FLCI contract is documented in docs/methodology/REGISTRY.md:L3624-L3672, including the nested optimizer, R parity note, and analytical folded-normal deviation from R.
    Concrete fix: None required.

Code Quality

  • Severity: None
    Impact: No code was changed.
    Concrete fix: None required.

Performance

  • Severity: None
    Impact: No runtime path was changed. The Rust nondeterminism item removal is consistent with the deterministic first-appearance cluster remap in rust/src/linalg.rs:L352-L360 and regression coverage in tests/test_rust_backend.py:L3627-L3647.
    Concrete fix: None required.

Maintainability

  • Severity: P3
    Impact: tests/test_wild_bootstrap.py:L1509-L1514 still says “see the TODO row on rust solve_ols nondeterminism,” but this PR deletes that row. This is a stale breadcrumb, not a correctness issue.
    Concrete fix: Update the comment to point to tests/test_rust_backend.py::TestClusterVcovDeterminism or remove the TODO-row reference.

Tech Debt

  • Severity: None
    Impact: Removing completed Actionable Backlog rows is consistent with the TODO policy; no active untracked correctness debt is introduced by the diff.
    Concrete fix: None required.

Security

  • Severity: None
    Impact: No secrets, PII, executable code, or security-sensitive paths were changed.
    Concrete fix: None required.

Documentation/Tests

  • Severity: P3
    Impact: No tests are necessary for this TODO-only change, but the stale comment noted above should be cleaned up for documentation consistency.
    Concrete fix: Update tests/test_wild_bootstrap.py:L1511-L1512 to reference the deterministic regression test instead of the removed TODO row.

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: d302033a3b4a5a7d10f42f8cf2b2ed7f1ddb9bea


Overall Assessment

Looks good — no unmitigated P0/P1 findings. This PR only removes completed TODO rows and adds a registry note for the resolved Rust clustered-vcov determinism issue.

Executive Summary

  • No estimator code, math, weighting, variance/SE logic, assumptions, or defaults are changed.
  • The HonestDiD Δ^SD optimal-FLCI TODO removal is consistent with the existing Methodology Registry documentation at docs/methodology/REGISTRY.md:L3626-L3667.
  • The Rust clustered-vcov TODO removal is supported by the new registry note at docs/methodology/REGISTRY.md:L82 and existing determinism coverage in tests/test_rust_backend.py::TestClusterVcovDeterminism.
  • Prior P3 stale-comment finding remains: one test comment still references the deleted TODO row.
  • One new P3 documentation nit: the registry says “50 identical calls,” while the tests currently loop 20 times per case.

Methodology

  • Severity: None
    Impact: No methodology behavior changed. The affected references are documentation/bookkeeping only: clustered-vcov determinism for shared OLS variance paths and HonestDiD Δ^SD optimal FLCI tracking. The documented HonestDiD FLCI implementation remains covered by the registry’s R-faithful nested optimizer note.
    Concrete fix: None required.

Code Quality

  • Severity: None
    Impact: No code paths were changed.
    Concrete fix: None required.

Performance

  • Severity: None
    Impact: No runtime behavior changed. Removing the completed Rust nondeterminism backlog row is consistent with the first-appearance cluster remap documented in docs/methodology/REGISTRY.md:L82.
    Concrete fix: None required.

Maintainability

  • Severity: P3
    Impact: Prior finding remains unresolved: tests/test_wild_bootstrap.py:L1509-L1514 still points to “the TODO row on rust solve_ols nondeterminism,” but this PR deletes that row. This is stale provenance only, not a correctness issue.
    Concrete fix: Update the comment to reference docs/methodology/REGISTRY.md or tests/test_rust_backend.py::TestClusterVcovDeterminism.

  • Severity: P3
    Impact: docs/methodology/REGISTRY.md:L82 says “50 identical calls are bit-identical,” but tests/test_rust_backend.py::TestClusterVcovDeterminism uses 20 repeats in each determinism test. The claim is directionally correct but not exactly aligned with the regression.
    Concrete fix: Change “50” to “repeated” or adjust the test loops to 50.

Tech Debt

  • Severity: None
    Impact: Removing completed Actionable Backlog rows is consistent with the project’s TODO policy and does not create untracked correctness debt.
    Concrete fix: None required.

Security

  • Severity: None
    Impact: No secrets, PII, executable code, or security-sensitive behavior changed.
    Concrete fix: None required.

Documentation/Tests

  • Severity: P3
    Impact: No tests are required for this TODO/registry-only PR. The two documentation polish items above are non-blocking.
    Concrete fix: Clean up the stale test comment and align the registry’s repeat-count wording with the actual regression test.

igerber and others added 3 commits July 9, 2026 08:59
- Rust clustered-vcov run-to-run nondeterminism: root-caused and fixed
  by #653 (SipHash HashMap iteration order -> first-appearance remap;
  50 identical calls now bit-identical). The row deferred its own
  deletion until the fix merged; #653 merged 2026-07-08.
- HonestDiD Delta^SD optimal-FLCI center parity: the row was already
  marked LANDED (SE-audit B2b, #635) with Effort 'Done' - it documented
  completed work, not remaining work.

No code changes; CHANGELOG untouched per convention for TODO-hygiene
commits (the underlying fixes carried their own entries).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…w P2)

The TODO-row deletion left no file-visible resolution trail; the
REGISTRY now documents the root cause (SipHash HashMap iteration
order), the fix (first-appearance aggregation, #653), and the
regression tests.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…m (review P3s)

The wild-bootstrap chunking comment pointed at the TODO row this PR
deletes; it now cites the REGISTRY determinism note + the
TestClusterVcovDeterminism regression. The REGISTRY '50 identical
calls' claim now distinguishes the in-fix 50-call probe from the
shipped 20-repeat regression loops.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@igerber igerber force-pushed the chore/todo-resolved-row-cleanup branch from d302033 to 1935c8d Compare July 9, 2026 13:00
@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: 1935c8da53a894fc92539b35815cd29484748d1c


Overall Assessment

Looks good — no unmitigated P0/P1 findings.

Executive Summary

  • No estimator code, math, weighting, SE/variance logic, assumptions, or defaults changed.
  • The removed Rust clustered-vcov TODO row is now covered by the registry determinism note at docs/methodology/REGISTRY.md:L82.
  • The shipped determinism regression matches the registry wording: 20 repeats per id layout in tests/test_rust_backend.py:L3627-L3676.
  • The removed HonestDiD Δ^SD optimal-FLCI row is covered by the R-faithful optimizer registry note at docs/methodology/REGISTRY.md:L3626-L3667.
  • Prior P3 comments are resolved: the wild-bootstrap comment no longer references the deleted TODO row, and the registry now distinguishes the 50-call probe from 20-repeat shipped tests.

Methodology

  • Severity: None
    Impact: Documentation/bookkeeping only. The affected methods are shared OLS clustered vcov documentation and HonestDiD Δ^SD FLCI tracking; no implementation behavior changed. The HonestDiD optimizer parity is documented in the registry, and the separate Δ^RM ARP deferred work remains tracked in TODO.md:L74.
    Concrete fix: None required.

Code Quality

  • Severity: None
    Impact: No code paths changed.
    Concrete fix: None required.

Performance

  • Severity: None
    Impact: No runtime or algorithmic behavior changed. The deleted Rust nondeterminism backlog row is supported by the registry note and existing regression tests.
    Concrete fix: None required.

Maintainability

  • Severity: None
    Impact: The previously stale comment in tests/test_wild_bootstrap.py:L1509-L1515 now points to the registry and TestClusterVcovDeterminism instead of the deleted TODO row.
    Concrete fix: None required.

Tech Debt

  • Severity: None
    Impact: Removing completed Actionable Backlog rows does not create untracked correctness debt. Remaining related deferred work is still represented in TODO.md, including the separate HonestDiD Δ^RM ARP item.
    Concrete fix: None required.

Security

  • Severity: None
    Impact: No secrets, PII, executable code, or security-sensitive behavior changed.
    Concrete fix: None required.

Documentation/Tests

  • Severity: None
    Impact: The registry note at docs/methodology/REGISTRY.md:L82 now aligns with tests/test_rust_backend.py:L3645-L3676: it describes a 50-call development probe and the shipped 20-repeat regressions separately. No additional tests are needed for this docs/TODO hygiene PR.
    Concrete fix: None required.

@igerber igerber added the ready-for-ci Triggers CI test workflows label Jul 9, 2026
@igerber igerber merged commit 4032346 into main Jul 9, 2026
31 of 32 checks passed
@igerber igerber deleted the chore/todo-resolved-row-cleanup branch July 9, 2026 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-ci Triggers CI test workflows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant