Conversation
Overall AssessmentExecutive Summary
MethodologyP3 — Documented Meridian pooled-SD policy Impact: Concrete fix: None required. No unmitigated methodology findings PyMC-Marketing’s lift-test API consumes Code QualityP1 — Inline inference computation in new test helper Location: Impact: The new helper constructs Concrete fix: import from diff_diff.utils import safe_inference
t_stat, p_value, conf_int = safe_inference(att, se)
return DiDResults(
att=att,
se=se,
t_stat=t_stat,
p_value=p_value,
conf_int=conf_int,
...
)PerformanceNo findings. The exporters are small linear passes over experiment rows; no obvious performance issue is introduced. MaintainabilityNo blocking findings. The new module is cohesive, has explicit validation helpers, and keeps MMM package dependencies out of runtime imports. Tech DebtP3 — Deferred MMM interop items are properly tracked Impact: Missing Meridian Concrete fix: None required for this PR. SecurityNo findings. I did not see secrets, credentials, or unsafe external execution paths in the changed code. Documentation/TestsP3 — Meridian Location: Impact: Meridian docs note that JAX users need Concrete fix: Optional: add a Test execution I attempted to run Path to Approval
|
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment✅ Looks good — no unmitigated P0/P1 findings. Executive Summary
MethodologyP3 — Documented Meridian pooled-SD policy Severity: P3 informational Impact: Concrete fix: None required. No unmitigated methodology findings PyMC-Marketing documents a lift-test frame with Code QualityNo findings. Re-review note: the previous P1 inline inference anti-pattern is resolved at PerformanceNo findings. The exporters are linear over the result rows and do not introduce heavy runtime dependencies. MaintainabilityNo findings. The new module is cohesive and keeps PyMC-Marketing/Meridian as output contracts rather than runtime dependencies. Tech DebtP3 — Deferred MMM interop follow-ups are tracked Severity: P3 informational Impact: Meridian Concrete fix: None required for this PR. SecurityNo findings. I did not see secrets, credential handling, unsafe subprocess execution, or network calls in the changed code. Documentation/TestsNo blocking findings. The new tests cover schema emission, scaling, wrong-sign policy, empty/drop-all handling, NaN/SE rejection, count handling, lognormal moment round-trip, pooling, channel-scoped code generation, and cross-estimator duck typing. Test execution attempted: Result: not run because |
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment⛔ Blocker — one newly identified P0 silent scaling error remains. Executive Summary
MethodologyP0 — [Newly identified] Staggered ATTs use an invalid common-duration multiplierSeverity: P0 Impact: Both exporters compute
where each cohort contributes once per eligible post-treatment period ( The MMM Registry repeats the common-duration formula but does not label this incompatibility as a Note/Deviation; the temporal-export TODO does not mitigate silent incorrect scalar output. Concrete fix: Fail closed for staggered result surfaces unless an exact positive aggregation scale or explicit aggregate effect-and-SE pair is supplied. Alternatively, derive the scale from the same eligible cohort-time weights used by the estimator. Document that automatic multiplication requires an additive level-scale outcome and a common treatment window. P3 — Documented Meridian pooled-SD policySeverity: P3 informational Impact: Concrete fix: None required; this deviation is explicitly labeled Code QualityP2 —
|
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment⛔ Blocker — two P0 silent scaling errors remain. Executive Summary
MethodologyP0 —
|
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment⛔ Blocker — one P0 silent estimand-scaling error remains. Executive Summary
MethodologyP0 — [Newly identified] TWFE is misclassified as 2×2 DiDSeverity: P0 Impact: This contradicts the Registry’s “verified” 2×2 classification ( Concrete fix: Dispatch on explicit estimator/estimand provenance, not the result class name. Add a stable marker distinguishing P3 — Documented numerical policiesSeverity: P3 informational Impact: The balanced-common-window assumption and independent-experiment ROI-SD pooling are documented under Concrete fix: None required. Code QualityP2 —
|
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment⛔ Blocker — three newly identified P0 correctness paths remain. Executive Summary
MethodologyP0 — [Newly identified] Unbalanced MultiPeriodDiD silently violates the scaling assumptionImpact: The Registry permits unbalanced The MMM Registry notes the balanced-panel assumption ( Concrete fix: Stamp balance/post-period treated-count provenance on P0 — [Newly identified] Survey-weighted Callaway–Sant’Anna has no valid integer
|
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment✅ Looks good — all prior P0 findings are resolved; no unmitigated P0/P1 findings remain. Executive Summary
MethodologyP3 — Documented independent-experiment variance policyImpact: Pooled ROI uncertainty omits covariance between experiments, potentially understating uncertainty for shared controls/windows. This is explicitly documented in Concrete fix: None required; use Code QualityP2 — Extreme arithmetic does not consistently follow the validation contractImpact: Concrete fix: Require finite post-test spend and compute pooled SD with a scaled norm such as PerformanceNo findings. MaintainabilityP2 — Documentation IA regression guards were removedImpact: Deleting Concrete fix: Restore the structural test and CI step, or replace them with equivalent checks. Tech DebtP3 — Result-derived aggregation remains trackedImpact: Estimator-owned aggregation and Meridian mask/tutorial helpers are recorded in Concrete fix: None required for this PR. SecurityNo findings. Documentation/TestsP2 — Geo example assigns an aggregate SDID ATT to one geoImpact: The examples fit SDID over a panel but label the resulting aggregate ATT as Concrete fix: State that the panel has exactly one treated geo, or omit the geo dimension and use aggregate-matched spend/effect inputs. P3 — Callaway workflow demonstrates unsupported headline scalingImpact: Concrete fix: Replace this test with caller-supplied known totals and SEs until estimator-owned aggregation exists. |
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment✅ Looks good — no unmitigated P0/P1 findings. Executive Summary
MethodologyP3 — Documented independent-experiment variance policyImpact: Pooled ROI uncertainty omits covariance across experiments and may be anti-conservative for overlapping samples/windows. Concrete fix: None required; this is documented in Code QualityP2 — Extreme ROI coefficient-of-variation raises raw
|
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall AssessmentExecutive Summary
MethodologyP1 — [Newly identified] Wrong-sign detection can underflowLocation: Impact: Concrete fix: Compare signs directly, for example P3 — Documented independent-experiment variance policyLocation: Impact: Pooled uncertainty omits covariance and may be anti-conservative for overlapping experiments. Concrete fix: None required; the deviation and Code QualityNo additional findings. The prior extreme-ROI PerformanceNo findings. MaintainabilityNo findings. Tech DebtP3 — Follow-up automation is trackedImpact: Result-derived scaling and Meridian mask construction remain manual. Concrete fix: None required; tracked in SecurityNo findings. Documentation/TestsThe new numerical regression tests cover the prior finding. Runtime tests could not run because required dependencies are unavailable; syntax and diff checks passed. Path to Approval
|
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment✅ Looks good — the previous P1 is resolved; no unmitigated P0/P1 findings remain. Executive Summary
MethodologyP3 — Documented independent-experiment variance policyLocation: Impact: Covariance between overlapping experiments is omitted, potentially understating uncertainty. Concrete fix: None required; the deviation and Code QualityP2 — Empty calibration expression generates invalid PythonLocation: Impact: Concrete fix: Reject blank strings, ideally validating the supplied expression with PerformanceNo findings. MaintainabilityNo findings. Tech DebtP3 — Follow-up automation remains trackedLocation: Impact: Calibration-mask construction and result-derived scaling remain manual. Concrete fix: None required; both limitations are tracked. SecurityNo findings. Documentation/TestsNo findings. The prior underflow case is now covered at |
…sts + Meridian ROI priors Interop builders that assemble Marketing Mix Model calibration inputs from experiment results. Explicit-in / validated-out: the caller supplies the already-scoped incremental outcome and its SE (aggregated to the population and window one MMM row represents), and diff-diff assembles the target schema, enforces each consumer's guards, converts to the lognormal parameterization, and pools. It does NOT rescale a headline ATT - that reconciliation needs the MMM's row granularity, time window, and outcome scale, which the exporter cannot see. Pure numpy/pandas; imports no MMM package; introspects no result object, so the module is purely additive (touches no estimator). - to_pymc_marketing_lift_test(channel, x, delta_x, delta_y, sigma, dims=, on_wrong_sign=): builds the lift-test DataFrame consumed by pymc-marketing's MMM.add_lift_test_measurements (prophetverse-compatible). Guards: sigma>0, delta_x!=0, x>=0, x+delta_x>=0, finite delta_y; wrong-sign (NonMonotonicError) AND zero-lift (degenerate for the Gamma lift likelihood) share one on_wrong_sign policy; dims reserved-column + shared-keys validation. - to_meridian_roi_prior(incremental_outcome, incremental_outcome_se, spend, parameter="roi_m"|"mroi_m", se_widening=): builds Google Meridian lognormal priors (MeridianROIPrior); mu/sigma match meridian's lognormal_dist_from_mean_std (verified 1.7.0), spend-weighted multi-experiment pooling, non-positive-ROI and finiteness guards, and a channel- and time-scoped .to_code() snippet that sets media_prior_type. roi_m vs mroi_m selects the estimand and each parameter's Meridian default for non-experiment channels. - 43 behavioral tests (schema, all guards, sign/zero policy, lognormal roundtrip via scipy, pooling, to_code scoping, realistic DiD/CS workflow) + end-to-end smoke verified against real pymc-marketing 0.19.4 (frame accepted into the model graph; wrong-signed row raises NonMonotonicError upstream). - Docs: REGISTRY interop section, api/mmm.rst, llms.txt + llms-full.txt, README line, references.rst, doc-deps.yaml, CHANGELOG; TODO/DEFERRED follow-up rows (Meridian mask builder, PR-B tutorial, post-4.0 result-derived scaling via results.aggregate()). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014wAAYSVo1Yf3XMcxNMrqsj
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment✅ Looks good — no unmitigated P0/P1 findings. Executive Summary
MethodologyP3 — Documented independent-experiment variance policyLocation: Impact: Correlation between overlapping experiments can understate pooled uncertainty. Concrete fix: None required; the deviation and Code QualityNo findings. The prior blank-expression issue is fixed at PerformanceNo findings. MaintainabilityNo findings. Tech DebtP3 — Follow-up automation is trackedLocation: Impact: Calibration masks and result-derived scaling remain caller-managed. Concrete fix: None required; both limitations are tracked. SecurityNo findings. Documentation/TestsNo findings. Invalid blank expressions are covered at |
1 participant
Summary
diff_diff/mmm.pyconverting DiD experiment results into MMM calibration inputs, with zero MMM-package dependency (pure numpy/pandas; emits plain DataFrames/dataclasses)to_pymc_marketing_lift_test(): emits the lift-test DataFrame consumed by PyMC-Marketing'sMMM.add_lift_test_measurements(channel/dims/x/delta_x/delta_y/sigma), with an explicit four-column unit contract (aggregate="mean"|"sum"),n_units=geo-to-national scaling,on_wrong_sign={"raise","drop","keep"}policy mirroring the upstream monotonicity check, anx + delta_x >= 0post-test-spend guard, and reserved-columndimsvalidation; schema is prophetverse-compatibleto_meridian_roi_prior(): emits Google Meridian lognormal ROI prior parameters (MeridianROIPrior) via Google's closed form, with spend-weighted multi-experiment pooling (per the Meridian FAQ recommendation, citing sec 3.4 of Google's MMM calibration whitepaper), anse_widening=transferability knob, and a channel-scoped.to_code()snippet helper (roi_mis per-channel in Meridian, so vector priors require the model channel order; scalar output needs explicitsingle_channel=True)att/sesurface every headline results class exposes; treated counts are never inferred from result metadata (n_treatedis a row count on several classes);n_units/n_periodsmust be positive integersdocs/api/mmm.rst, llms.txt + llms-full.txt sections, README one-liner, references.rst entries, doc-deps.yaml entry, CHANGELOG, TODO.md follow-up rows for documented deferrals (Meridianroi_calibration_periodmask, event-study temporal lift export, Robyn, GeoX)Methodology references (required if estimator / math changes)
meridian/model/prior_distribution.pyat 1.7.0 (lognormal closed form + per-channelroi_mbatch shape + defaultLogNormal(0.2, 0.9)); Meridian FAQ (spend-weighted average ROI for multiple experiments, citing sec 3.4 of Google's MMM calibration whitepaper); Zhou, Choe & Hetrakul (2023), Meta Marketing Science (calibration motivation)roi_sd = sqrt(sum((w_i * sd_i)^2))treats experiments as independent - this uncertainty propagation is this library's policy (the FAQ prescribes only the spend-weighted mean); documented with a**Note:**in REGISTRY.md, withse_widening=as the correlated-experiments mitigation.Validation
tests/test_mmm.py(new, 51 behavioral tests): exact schema emission, hand-computed scaling for mean/sum/n_units, wrong-sign policies incl. drop-all-raises, go-dark boundary (x + delta_x == 0passes, below raises), NaN/zero-SE rejection, broadcasting/empty-sequence/reserved-dims errors, scipy lognormal moment round-trip (1e-12), spend-weighted pooling math, channel-scope regression for.to_code()(experiment prior lands only in its channel slot), units-not-rows pin (no silentn_treatedfallback), and cross-estimator duck-typing contract (DifferenceInDifferences / SyntheticDiD / MultiPeriodDiD / CallawaySantAnna)MMM.add_lift_test_measurementsinto the model graph, and a wrong-signed row reproduced upstreamNonMonotonicError;mu/sigmaverified bit-identical to Meridian'slognormal_dist_from_mean_stdover 200 random draws. Tutorial notebook follows in PR-B.Security / privacy
🤖 Generated with Claude Code
https://claude.ai/code/session_014wAAYSVo1Yf3XMcxNMrqsj