docs: migrate narrative RST off fit-time aggregate= (sweep PR-A) - #763
Merged
Conversation
Migrates the narrative-docs half of the fit-time aggregate= teaching sweep
(TODO row, M-020 family) onto post-fit results.aggregate(type=):
choosing_estimator.rst drops its second full refit, python_comparison.rst
now mirrors the differences package's post-fit idiom, r_comparison.rst maps
1:1 onto R's three aggte() calls, and troubleshooting.rst's event-study-plot
remedy plots the aggregate('event_study') container with an explicit
reference_period. The troubleshooting bootstrap passage deliberately keeps
fit-time aggregate= with a comment naming it the documented until-4.0
exception (post-fit recompute levels raise on a bootstrapped fit; 'simple'
relays). The two rewritten blocks are now self-contained and removed from
_CONTEXT_DEPENDENT_SNIPPETS so the snippet harness actually executes them.
…ew R1) Verified by execution: the varying-base default's event-study container has no reference row (is_reference all False; e=-1 is an estimated pre-treatment effect), so plot_event_study(es, reference_period=-1) would renormalize around an estimate - the REGISTRY varying-base common-reference warning. The universal-base fit marks e=-1 as the true reference, keeping the section's specify-the-reference-explicitly lesson coherent.
…eview R2) On gapped period grids the universal-base positional reference can sit at an event time other than -1 (the reference_event_times multi-anchor case the common-reference guards exist for), so the example now inspects the container's is_reference row(s) and plots without a manual override.
Overall assessment: ✅ Looks goodExecutive summary
MethodologyNo findings. Code QualityNo findings. PerformanceNo findings. MaintainabilityNo findings. Tech Debt
SecurityNo findings. Documentation/Tests
|
…lock (CI review) The block constructs CallawaySantAnna but only imported plot_event_study - a copy-paste into a fresh session would NameError; the snippet harness masked it by preloading every public name.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment: ✅ Looks goodExecutive summary
MethodologyNo findings. This documentation-only change correctly describes Code QualityNo findings. PerformanceNo findings. MaintainabilityNo findings. Tech Debt
SecurityNo findings. Documentation/TestsNo findings. The prior |
… P3) The narrative-RST half this PR completes no longer reads as outstanding; the row now carries only the notebook half and the blocked API pages.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment: ✅ Looks goodExecutive summary
MethodologyNo findings. The documentation matches Code QualityNo findings. PerformanceNo findings. MaintainabilityNo findings. Tech DebtNo findings. SecurityNo findings. Documentation/TestsNo findings. The newly unskipped |
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
1 participant
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.
Summary
aggregate=teaching sweep (the 3.9-deprecated M-020 family) onto post-fitresults.aggregate(type=...):choosing_estimator.rstdrops its second full refit forresults.aggregate('event_study');python_comparison.rstnow mirrors thedifferencespackage's post-fit idiom it is paired against;r_comparison.rst's aggregation block maps 1:1 onto R's threeaggte()calls (simple/event_study/group; the fit-time'all'has no post-fit counterpart).troubleshooting.rst's event-study-plot remedy coherently: abase_period="universal"fit whoseaggregate('event_study')container is inspected for its actualis_referencerow(s) and plotted directly - no hard-codedreference_period=-1, because under the varying default every pre-period point is an estimated effect with no common anchor, and on gapped grids even the universal base can anchor at an event time other than -1 (the REGISTRY positional-base contract).aggregate='event_study'with a comment naming it the documented until-4.0 exception: post-fit recompute levels raise on a bootstrapped fit (staggered_results.pyfail-closed gate),'simple'relays. This matches the AI guides anddocs/migration-4.0.md.NameErrors before their teaching lines executed (r_comparison:block2,troubleshooting:block8); both are now self-contained and removed from_CONTEXT_DEPENDENT_SNIPPETS, sotests/test_doc_snippets.pyactually executes them. Block counts per file are unchanged, so the surviving positional allowlist keys still point at their intended blocks.[Unreleased]section with this entry. A follow-up PR (sweep PR-B) migrates the tutorial notebooks and lands after this one.Methodology references (required if estimator / math changes)
is_referencerow, so renormalizing a plot arounde=-1would shift every point).Validation
tests/test_doc_snippets.py- removedr_comparison:block2andtroubleshooting:block8from_CONTEXT_DEPENDENT_SNIPPETS(the rewritten blocks are self-contained and now execute under the harness). Fulltests/test_doc_snippets.py+tests/test_docs_ia.pypass locally (138 passed, 5 skipped), including the r_comparison argument-mapping-table gate.Security / privacy