Add ACA marketplace bronze-selection target ETL#618
Conversation
baogorek
left a comment
baogorek
left a comment
There was a problem hiding this comment.
Hi @daphnehanse11 , I'm going to let my Claude do the talking below, but the short of it is that there's a lot to do. I think Codex went for the quick win, and there's just not a quick win here.
the CMS data sourcing is thorough and the underlying goal of decomposing PTC into used vs. unused makes sense. However, I think
the approach needs to be restructured. The matrix builder should stay generic and not contain variable-specific logic, and the variables you're deriving
don't yet exist in the places they need to for calibration to actually work.
Here's the full path I'd suggest, roughly in dependency order:
1. policyengine-us: Add used_aca_ptc, unused_aca_ptc, and selects_bronze_marketplace_plan as real calculated variables with formulas and parameters. The
state-level bronze selection probabilities and price ratios from your CMS data become parameters there. Everything downstream depends on these existing
first.
2. ETL scripts (policy_data.db): Derive state-level calibration targets (e.g., total used PTC by state) from the CMS data and load them into the targets
database. That's where calibration targets live now.
3. enhanced_cps.py: Wire up the bronze plan selection so the legacy calibration pipeline has access to the new variables.
4. target_config.yaml: Add the new variable names so the unified matrix builder picks them up — no code changes to the builder itself, just config.
With this approach, the matrix builder never needs to know what these variables are. It just sees new names in the config and new rows in the database, same
as any other target.
I'd suggest starting with step 1 since everything else depends on it.
New "under construction" node type (amber dashed) for showing pipeline changes that are actively being developed: US: - PR #611: Pipeline orchestrator in Overview (Modal hardening) - PR #540: Category takeup rerandomization in Stage 2, extracted puf_impute.py + source_impute.py modules in Stage 4 - PR #618: CMS marketplace data + plan selection in Stage 5 UK: - PR #291: New Stage 9 — OA calibration pipeline (6 phases) - PR #296: New Stage 10 — Adversarial weight regularisation - PR #279: Modal GPU calibration nodes in Stages 6, 7, Overview Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
New "under construction" node type (amber dashed) for showing pipeline changes that are actively being developed: US: - PR #611: Pipeline orchestrator in Overview (Modal hardening) - PR #540: Category takeup rerandomization in Stage 2, extracted puf_impute.py + source_impute.py modules in Stage 4 - PR #618: CMS marketplace data + plan selection in Stage 5 UK: - PR #291: New Stage 9 — OA calibration pipeline (6 phases) - PR #296: New Stage 10 — Adversarial weight regularisation - PR #279: Modal GPU calibration nodes in Stages 6, 7, Overview Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
baogorek
left a comment
baogorek
left a comment
There was a problem hiding this comment.
@daphnehanse11 I'm requesting that this PR be refocused to the targets ETL and perhaps the ECPS logic. Please note that that this current PR will not affect the ECPS because it's not touching either loss.py or enhanced_cps.py. I don't think your coding agent was able to pick up on the two distinct paths.
I cannot approve the changes in unified_matrix_builder.py or publish_local_area.py. and I recommend that they be removed from the PR. Hard-coded variables in the matrix builder are what made the junkyard the junkyard. We need to do everything humanly (or codexly) possible to never, ever hard-code a variable in unified_matrix_builder.py.
It is possible that publish_local_area.py will need a small modification before this works in local area calibration. Once these targets are in, we can start building models locally and test out the changes. So, I really think this needs to be a two part process.
So if you want the ECPS to be improved, which will get you a benefit now, there needs to be a separate editing of loss.py or enhanced_cps.py in this PR. In that case, some CSVs are acceptable in the storage/calibraiton folder. If you only want better local area h5 calibration, then there should not be CSVs at all, with the exception of sources are not available for download online (like our national "Tips" target). Please see etl_medicaid.py for reference.
Note: the meaning of "ETL" is
E: Extract from the original source
T: Transform the data
L: Load the data into the database.
Forgive me from being tough on this PR: the target sourcing is excellent work. There is just a lot of risk in modifying some of these files.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
daphnehanse11
changed the title
baogorek
left a comment
baogorek
left a comment
There was a problem hiding this comment.
Please check the comments, but there are no blockers here.
baogorek
left a comment
baogorek
left a comment
There was a problem hiding this comment.
Oh @daphnehanse11 , I forgot! You have to put your etl python file in the Makefile so that make database will run it!
When I did this and tried to run it, I did get an error:
RuntimeError: ACA marketplace ETL requires policyengine-us variables that are not available in the current environment: selected_marketplace_plan_benchmark_ratio, used_aca_ptc
make: *** [Makefile:81: database] Error 1
|
Addressed the latest follow-up as well:
Also pushed the inline documentation/comment updates requested on the ETL file. |
baogorek
left a comment
baogorek
left a comment
There was a problem hiding this comment.
@daphnehanse11 thank you for addressing my comments. Everything looks good and make database runs fine locally for me. I understand this is a problem with the uv.lock, which should be easy to fix.
The next step will be to try calibrating in an actual model run! You'll do that by editing the target config. I would suggest that you (or someone else) fit the model once locally to ensure it doesn't take down calibration.
|
Please rebase from upstream/main (there are conflicts) and fix the uv.lock issue flagged in the last review so |
|
Status check from a rebase attempt against current Integration-tests failure — the red job was a pre-existing Census API flake unrelated to this PR ( Rebase conflicts —
If the intent of the refocused PR is ETL-only, those unified-matrix-builder additions should come out during the rebase. If they should stay, the PR description needs updating. Your call — I didn't force-push the rebase since it's an editorial decision on your work. Related follow-up: #800 (per-household |
daphnehanse11
force-pushed
the
codex/aca-marketplace-plan-selection
branch
from
eb1d7bf to
6ad2911
Compare
|
Rebased this PR onto current The diff is now ETL-only again:
I rebuilt the branch from a fresh
Validation on the rebased branch:
|
Review fixes from the standing review of PR 618: 1. P0 bug: bronze stratum constraints were inserted in the order ``state_fips, used_aca_ptc, selected_marketplace_plan_benchmark_ratio``, which SQLite's ``GROUP_CONCAT(DISTINCT ...)`` preserves insertion order for. That produced ``domain_variable = "used_aca_ptc,...``, but ``target_config.yaml:68`` expects the alphabetical form ``selected_marketplace_plan_benchmark_ratio,used_aca_ptc``. The rule didn't match, so the bronze target silently dropped out of the loss. Reorder the inserts and add a comment explaining why order matters. 2. Delete the now-dead ``etl_aca_agi_state_targets.py`` — it still used ``source="CMS Marketplace"`` (rejected by ``create_field_valid_values``) and the Makefile no longer invokes it. Redirect ``tests/integration/test_database_build.py`` to the new ``etl_aca_marketplace.py``. 3. Add a ValueError guard for corrupt source data (bronze APTC consumers exceeding total APTC consumers for any state). 4. Add the CMS Marketplace PUF URL to the ETL extract docstring so the input CSV is actually refetchable. 5. Expand the unit test file: add a real-CSV regression test (expects 27+ HC.gov states with bronze ≤ total and no SBM states leaking in) and a negative test for the new ValueError.
|
@codex please re-review. Addressed all prior feedback in commit 8fd8990:
Upstream variable landed in #801 (today), so the bronze stratum is now actually evaluable against current microdata. |
… test Codex review on 8fd8990 found two issues: 1. ``tests/integration/test_database_build.py::test_state_aca_and_agi_targets_loaded`` still asserted legacy ``aca_ptc`` / ``person_count`` / ``adjusted_gross_income`` state targets that the deleted ``etl_aca_agi_state_targets.py`` used to load, so it would fail against the rebuilt DB. Rename and rewrite it as ``test_state_marketplace_targets_loaded`` that asserts the new APTC and bronze-selection targets land with the canonical alphabetical ``domain_variable`` strings. 2. The previous constraint-insertion-order workaround relied on SQLite's ``GROUP_CONCAT(DISTINCT ...)`` preserving insertion order, which is undocumented. Add ``ORDER BY`` to the ``domain_variable`` aggregation in the ``stratum_domain`` view so the canonical form is enforced at the view level, regardless of how callers insert constraints. Drop the now-obsolete ordering comment in ``etl_aca_marketplace.py``.
…ity) The prior ``GROUP_CONCAT(DISTINCT ... ORDER BY ...)`` form requires SQLite >= 3.44 and failed on the Modal integration runner with ``sqlite3.OperationalError: near "ORDER": syntax error``. Replace with a correlated subquery that selects distinct constraint names ordered alphabetically and then concatenates them without an inner ORDER BY. Works on all supported SQLite versions and still produces the canonical form (e.g. ``selected_marketplace_plan_benchmark_ratio,used_aca_ptc``) regardless of constraint insertion order. Verified by running the real view against in-memory SQLite with non-alphabetical insert order; result matches the expected canonical string.
The deletion in 8fd8990 was too aggressive. That file loaded three distinct target families into the calibration DB: 1. state-level ``aca_ptc`` spending targets (sourced from ``aca_spending_and_enrollment_2024.csv``) 2. state-level ``person_count`` enrollment targets (same source) 3. state-level AGI bracket targets (sourced from ``agi_state.csv``) This PR adds *new* marketplace APTC-count and bronze-count targets but does not replace the ACA spending/enrollment or AGI targets. Without them the calibrator has nothing to pin state-level ACA PTC spending, and ``test_aca_calibration`` / ``test_sparse_aca_calibration`` fail with >500% state deviations. Restore the file verbatim from the pre-deletion state, keep the ``CMS Marketplace`` source string it uses (re-added to the ``create_field_valid_values`` allowlist alongside the newer ``CMS 2024 OEP state metal status PUF`` source the marketplace ETL uses), re-add the Makefile invocation, and put its entry back in the integration-build script list ahead of the marketplace ETL. Keep the new ``test_state_marketplace_targets_loaded`` as a peer to the restored ``test_state_aca_and_agi_targets_loaded``. The long-term cleanup (migrating the spending/enrollment targets into the marketplace ETL or deprecating them) is a follow-up.
…ce-plan-selection
3 participants
Summary
This PR has been refocused to the ACA marketplace targets ETL path.
It now:
policyengine_us_data/db/etl_aca_marketplace.pyto transform 2024 CMS state metal status data into state-level ACA marketplace targetspolicyengine_us_data/storage/calibration_targets/aca_marketplace_state_metal_selection_2024.csvWhy
This follows review feedback to keep
unified_matrix_builder.pygeneric and to avoidpublish_local_area.py/ calibration-plumbing changes in this PR.Review fixes (commit 8fd8990)
state_fips, used_aca_ptc, selected_marketplace_plan_benchmark_ratio, which SQLite'sGROUP_CONCAT(DISTINCT ...)preserves insertion order for. That produceddomain_variable = "used_aca_ptc,selected_marketplace_plan_benchmark_ratio", buttarget_config.yaml:68expects alphabeticalselected_marketplace_plan_benchmark_ratio,used_aca_ptc. The rule didn't match, so the bronze target silently dropped out of the loss. Now reordered and documented.etl_aca_agi_state_targets.py(still used bannedsource="CMS Marketplace"; Makefile already dropped its invocation); retargetedtests/integration/test_database_build.pyto the new ETL.ValueErrorguard for corrupt source data (bronze > total).Upstream variable now exists
When this PR was opened, the
selected_marketplace_plan_benchmark_ratiovariable was not yet populated on CPS data. #801 (merged 2026-04-20) now imputes it from reported premiums, so the bronze stratum is actually evaluable against current microdata.Out of Scope
This PR does not:
unified_matrix_builder.pypublish_local_area.pyenhanced_cps.pyorloss.pyThose downstream pieces can follow separately.
Validation
pytest tests/unit/test_aca_marketplace_targets.py -v— 3 tests pass (original happy path, real-CSV regression, negative bronze-exceeds-total case)ruff format --checkandruff checkclean on touched files