Skip to content

Remove adds from is_pregnant#7719

Open
baogorek wants to merge 3 commits intomainfrom
remove-is-pregnant-adds
Open

Remove adds from is_pregnant#7719
baogorek wants to merge 3 commits intomainfrom
remove-is-pregnant-adds

Conversation

@baogorek
Copy link
Collaborator

@baogorek baogorek commented Mar 6, 2026

Summary

  • Remove adds = ["current_pregnancies"] from is_pregnant, making it a pure input variable rather than one derived from current_pregnancies
  • Fix IL MPE unit tests to use is_pregnant directly as input instead of relying on the adds derivation from current_pregnancies
  • Fix IL MPE integration tests to provide is_pregnant: true alongside current_pregnancies for pregnant persons

Context

is_pregnant is a direct demographic input ("are you pregnant?"), not something derived from current_pregnancies ("how many babies are you expecting?"). The adds line caused policyengine-us-data's _drop_formula_variables to drop stochastically imputed is_pregnant values during CPS enhancement.

Closes PolicyEngine/policyengine-us-data#576

Test plan

  • IL MPE eligible tests pass (4/4)
  • IL MPE integration tests pass (5/5)
  • IL MPE income eligible tests pass (3/3)
  • tax_unit_medicaid_income_level tests pass (4/4)

🤖 Generated with Claude Code

@baogorek baogorek force-pushed the remove-is-pregnant-adds branch from 7ae64c8 to 76b7262 Compare March 6, 2026 20:23
@PavelMakarchuk
Copy link
Collaborator

Thinking that we will need to at least adjust the Illinois program which was dependent on the number of pregnancies

@codecov
Copy link

codecov bot commented Mar 6, 2026

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 93.33%. Comparing base (63a7861) to head (e9bba9f).
⚠️ Report is 29 commits behind head on main.

Files with missing lines Patch % Lines
...ousehold/demographic/person/current_pregnancies.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##              main    #7719      +/-   ##
===========================================
- Coverage   100.00%   93.33%   -6.67%     
===========================================
  Files            5        2       -3     
  Lines           69       15      -54     
  Branches         2        0       -2     
===========================================
- Hits            69       14      -55     
- Misses           0        1       +1     
Flag Coverage Δ
unittests 93.33% <66.66%> (-6.67%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

@baogorek baogorek force-pushed the remove-is-pregnant-adds branch from 76b7262 to b47bed7 Compare March 6, 2026 22:35
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests take current_pregnancies as inputs and outputs, I dont think they are effective but our convention is to not write unit tests for variables without formulas

Make is_pregnant a pure input variable by removing adds = ["current_pregnancies"].
Add defined_for = "is_pregnant" to current_pregnancies so it is scoped to
pregnant persons. Add tests for current_pregnancies with defined_for behavior.

Closes PolicyEngine/policyengine-us-data#576

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@baogorek baogorek force-pushed the remove-is-pregnant-adds branch from b47bed7 to 70cc92d Compare March 9, 2026 15:45
@baogorek
Copy link
Collaborator Author

baogorek commented Mar 9, 2026

@PavelMakarchuk
CC talking:

Thanks for the review!

**On the test file**: You're right — removed the `current_pregnancies.yaml` tests since it's a variable without a formula. The `defined_for` behavior is already covered by the IL MPE integration tests.

**On the Illinois program**: The IL MPE programs (`il_mpe_eligible`, `il_tanf_demographic_eligible_person`, `il_fpp_eligible`, etc.) all read `is_pregnant` directly — none read `current_pregnancies`. The `current_pregnancies` variable flows into Medicaid household size via the federal `tax_unit_medicaid_income_level` formula, which is unchanged. The IL MPE integration tests already provide both `is_pregnant: true` and `current_pregnancies` for pregnant persons and all 12 tests pass.

Hopefully we're just logically switching the order here.

@PavelMakarchuk
Copy link
Collaborator

@hua7450 can you take a look to make sure we dont break any partner programs here

@hua7450
Copy link
Collaborator

hua7450 commented Mar 9, 2026

I will write some test cases to verify. Just to make sure our api partners don't need to adjust anything.

Add a formula to current_pregnancies that returns 1, gated by
defined_for = "is_pregnant". This means setting is_pregnant = True
automatically gives current_pregnancies = 1 without needing to
specify it explicitly. Users can still override (e.g., twins = 2).

Update tax_unit_medicaid_income_level tests to add is_pregnant: true
for pregnant persons and add a triplets test case.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@hua7450
Copy link
Collaborator

hua7450 commented Mar 9, 2026

Why current_pregnancies needs a formula (not just default_value)

We added def formula: return 1 to current_pregnancies so that setting is_pregnant = True automatically gives current_pregnancies = 1 without the user needing to specify it. This means:

  • Before this PR: You had to set current_pregnancies to drive both variables (since is_pregnant was derived from it via adds)
  • After this PR: Just set is_pregnant = True and current_pregnancies defaults to 1. Override explicitly for twins/triplets.

We use a formula instead of default_value = 1 because default_value applies to all persons regardless of defined_for — every person would get current_pregnancies = 1, even non-pregnant ones. The formula is gated by defined_for = "is_pregnant", so it only returns 1 for pregnant persons and 0 for everyone else.

PolicyEngine Core behavior note

There's a Core-level behavior worth documenting: when current_pregnancies is set as input on any person in a simulation, the formula is skipped for all persons. PolicyEngine Core uses a single array per variable per period — setting input on any entity initializes the whole array from inputs (with defaults for unset entities), bypassing the formula entirely.

Example — multiple pregnant people, only one sets current_pregnancies:

input:
  people:
    p1:
      is_pregnant: true
      # current_pregnancies not set — expects formula to default to 1
    p2:
      is_pregnant: true
      current_pregnancies: 3  # explicitly set for triplets
    p3:
      age: 35
      # not pregnant

output:
  is_pregnant:          [true, true, false]
  current_pregnancies:  [0,    3,    0]      # ← p1 got 0, expected 1!

Because p2 sets current_pregnancies: 3, Core treats the entire variable as "has input" and skips the formula for everyone. p1 gets the default (0) instead of the formula result (1).

Workaround: If any person in the simulation sets current_pregnancies, all pregnant persons must set it explicitly:

input:
  people:
    p1:
      is_pregnant: true
      current_pregnancies: 1  # must set explicitly now
    p2:
      is_pregnant: true
      current_pregnancies: 3

output:
  current_pregnancies:  [1, 3, 0]  # ✅ correct

In practice this mainly affects test cases. In the web app and microsimulation, users typically set only is_pregnant, so the formula runs and defaults everyone to 1 correctly.

Simpler equivalent: is_pregnant (bool) maps directly to
current_pregnancies (1 for pregnant, 0 for not).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.

is_pregnant stochastic draw silently dropped by extended_cps formula filter

3 participants