Skip to content

Add HierarchicalLinearRegression for Hierarchical DiD#860

Open
jsakv wants to merge 6 commits into
pymc-labs:mainfrom
jsakv:hdid-model
Open

Add HierarchicalLinearRegression for Hierarchical DiD#860
jsakv wants to merge 6 commits into
pymc-labs:mainfrom
jsakv:hdid-model

Conversation

@jsakv

@jsakv jsakv commented Apr 20, 2026

Copy link
Copy Markdown
Contributor

This PR adds the HierarchicalLinearRegression with random intercepts and random slopes.

The model takes both a fixed-effect design matrix and a random-effect design matrix, similar to Bambi or Statmodel’s linear mixed effect models.

This PR is part of issue #656 Hierarchical DiD experiment resolution.

@github-actions

Copy link
Copy Markdown
Contributor

👋 Welcome to CausalPy, @jsakv!

Thank you for opening your first pull request! We're excited to have you contribute to the project. 🎉

Here are a few tips to help your PR get merged smoothly:

  • ✅ Make sure all CI checks pass (tests, linting, type checking)
  • 📝 Run prek run --all-files locally before pushing
  • 📖 Check our Contributing Guide for more details

A maintainer will review your changes soon. Thanks for helping make CausalPy better! 🚀


💼 LinkedIn Shoutout: Once your PR is merged, we'd love to give you a shoutout on LinkedIn to thank you for your contribution! If you're interested, just drop your LinkedIn profile URL in a comment below.

@read-the-docs-community

read-the-docs-community Bot commented Apr 21, 2026

Copy link
Copy Markdown

Documentation build overview

📚 causalpy | 🛠️ Build #32918031 | 📁 Comparing a6d724d against latest (78ec91d)

  🔍 Preview build  

283 files changed · + 98 added · ± 184 modified · - 1 deleted

+ Added

± Modified

- Deleted

@jsakv jsakv force-pushed the hdid-model branch 2 times, most recently from 9da4533 to f9f0946 Compare April 22, 2026 12:59
@jsakv

jsakv commented Apr 22, 2026

Copy link
Copy Markdown
Contributor Author

Pending on #852 fix to pass CI checks

@drbenvincent

Copy link
Copy Markdown
Collaborator

This PR's failing test (3.11) / test (3.14) jobs are caused by test_panel_regression_plot_unit_effects (TypeError: only 0-dimensional arrays can be converted to Python scalars), which was fixed in #853 (now merged to main). Updating this branch from main should turn CI green — no code changes needed here.

@codecov

codecov Bot commented Apr 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.37888% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 95.12%. Comparing base (c107d4e) to head (a6d724d).

Files with missing lines Patch % Lines
causalpy/tests/test_pymc_models.py 98.86% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #860      +/-   ##
==========================================
+ Coverage   95.07%   95.12%   +0.05%     
==========================================
  Files          87       87              
  Lines       13701    13862     +161     
  Branches      812      815       +3     
==========================================
+ Hits        13026    13186     +160     
  Misses        479      479              
- Partials      196      197       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jsakv jsakv marked this pull request as ready for review May 18, 2026 14:53
@jsakv jsakv marked this pull request as draft May 18, 2026 14:53
@jsakv jsakv marked this pull request as ready for review May 18, 2026 15:29
@jsakv

jsakv commented May 30, 2026

Copy link
Copy Markdown
Contributor Author

The PR is ready for review. Let me know what you think @drbenvincent!

Key points:

  • I have aligned prior setting with a pattern closer to the InstrumentalVariableRegression pattern for protecting against silently mixing partial user priors with default variance-component priors. 

  • I have still implemented the prior override logic, just being a bit stricter:
    • If priors is None: use HLR default_priors.
    • If priors: require the user to provide all priors for the chosen parameterization.

      
Happy to add data-driven priors later, I would like to get the HDID experiment out first!

@drbenvincent

Copy link
Copy Markdown
Collaborator

Thanks for this @jsakv, and apologies for the delayed review!

One question before the detailed feedback: do you intend to continue with the experiment wrapper + the marketing-campaign notebook (from #656) in this same PR, or were you planning to land those as separate follow-up PRs? My preference would be to keep them together in this PR if that works for you — it's much easier to review the model, the experiment that uses it, and a worked example as a single coherent unit than as several independent PRs where the model lands without a user-facing entry point.

With that said, here's a review of the progress so far. Overall this is a solid foundation and I'm happy with the direction.

What's working well

  • The model math and shapes check out for both the centered and non-centered parameterizations (mu_fixed = X·βᵀ, mu_random = sum(β_random[group] · Z, axis=2), and β_random = β_group · σ_random in the non-centered path).
  • The prior-validation design is the right call. Validating against the user-supplied priors (rather than the defaults-merged dict) is exactly what prevents silently mixing partial user priors with default variance-component priors, and it mirrors the InstrumentalVariableRegression pattern nicely. The tests cover each missing-prior case for both parameterizations.
  • Good test coverage of parameter recovery, the fit/predictive path, and both parameterizations. codecov/patch is green.

Minor

  • The ValueError in build_model raises two near-identical lines:
    raise ValueError(
        f"Missing required priors for {parameterization} parameterization: {missing}.\n"
        f"Missing required {prior_source} priors for {parameterization} parameterization: {missing}."
    )
    The second line subsumes the first — I'd collapse this to a single message (the test regex will still match).

CI

Happy to defer the data-driven priors to a later PR as you suggested. Let me know your thoughts on keeping the experiment + notebook in this PR and I'll do a fuller review once those land.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants