Skip to content

Add explicit input / value-state provenance helpers#516

Open
edithatogo wants to merge 3 commits into
PolicyEngine:masterfrom
edithatogo:feature/explicit-input-value-state
Open

Add explicit input / value-state provenance helpers#516
edithatogo wants to merge 3 commits into
PolicyEngine:masterfrom
edithatogo:feature/explicit-input-value-state

Conversation

@edithatogo

@edithatogo edithatogo commented Jul 9, 2026

Copy link
Copy Markdown

What's changed

Closes #513.

Adds a narrow, non-breaking provenance query over input tracking that PolicyEngine already maintains:

  • Holder.is_input(period, branch_name="default");
  • Simulation.is_input(variable_name, period);
  • Simulation.get_value_state(variable_name, period), returning explicit or default.

Explicit zero remains distinguishable from an omitted/defaulted value. Formula cache writes remain non-inputs. Calculation defaults and formula semantics are unchanged.

Verification

  • Regression tests cover explicit zero, omitted input, situation-dict input, cache writes, branch behavior, and period handling.
  • pytest tests/core/test_holders.py passes locally (25 tests at the recorded revision).
  • Ruff check and format pass for touched files.
  • Changelog fragment added.
  • make format && make documentation awaits the upstream workflow run.
  • Issue linked and project item updated.

Compatibility and limits

This is an opt-in query API, not unknown-value propagation or an engine-wide missingness mode. Existing calculations continue to default omitted inputs exactly as before.

Maintainer action required

GitHub is holding workflow execution for this first-time fork contribution. Please approve the non-secret workflow run and review the public API naming.

@MaxGhenis MaxGhenis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for this — it's a careful, minimal solve for a real gap (the missing-vs-explicit-zero ambiguity you documented in #513 for SNAP screeners), and I especially like that it adds zero new tracking state. Reviewed in full and ran the suite locally.

What works well

  • Reuses existing provenance. is_input reads the _user_input_keys set that set_input already maintains for reform/cache invalidation, so there's no new bookkeeping and no calculation-path cost.
  • Correctly excludes cache writes. _user_input_keys is only populated inside set_input's _set path (gated on _user_input_contexts), so put_in_cache writes are not treated as inputs — verified by the new test.
  • The explicit-zero case is exactly right. set_input(0) reads as explicit, missing reads as default, and the value still defaults to zero in formulas — a safe opt-in query layer with no behavior change.
  • Verified locally: tests/core/test_holders.py → 25 passed; no regressions in test_simulations.py.

Suggestions (non-blocking)

  1. Reuse Simulation._get_visible_branch_names() for branch inheritance. Holder.is_input re-implements the ancestor-plus-default walk by hand, but Simulation already has the canonical version: _get_visible_branch_names() returns [self.branch_name, …ancestors…, "default"] deduped, and _get_exportable_input_periods already uses it for exactly this membership question. Delegating to it keeps the two input-provenance paths from drifting. It also removes a latent coupling: the current manual walk uses simulation.parent_branch regardless of the branch_name argument, which is only correct when branch_name == simulation.branch_name (the only path wired today, via Simulation.is_input).
  2. ETERNITY periods — worth a check. The reform-reset logic notes that ETERNITY-defined variables canonicalize every period to a single storage key, but is_input normalizes the lookup with periods.period(period) without that canonicalization. So is_input(concrete_period) on an ETERNITY variable could miss a recorded input. Screener monetary inputs are MONTH/YEAR, so this isn't urgent — worth either a guard or a note on the method.
  3. Document the intended value-state vocabulary. get_value_state returns "explicit" / "default" today, and the docstring already flags that computed values read as "default" for now. Naming the planned future states (e.g. "computed", and an "unknown" for the "cannot determine" mode you raised in #513) would let downstream consumers code against a stable vocabulary as this grows.

CI status

No checks have run yet — the status rollup is empty because this is a first-time fork contribution and GitHub is holding the workflow runs for maintainer approval (not a pass/fail). The touched code is covered by tests/core unit tests that need no repository secrets, so Lint / check-changelog / Test should go green once a maintainer approves the run.

This is a solid foundation for input-level provenance — happy to see it land after the _get_visible_branch_names() reuse.

@codecov

codecov Bot commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 79.59184% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.23%. Comparing base (c94a745) to head (b5750bb).
⚠️ Report is 37 commits behind head on master.

Files with missing lines Patch % Lines
policyengine_core/holders/holder.py 47.36% 8 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #516      +/-   ##
==========================================
+ Coverage   84.69%   86.23%   +1.54%     
==========================================
  Files         223      230       +7     
  Lines       11830    12823     +993     
  Branches     1169     1248      +79     
==========================================
+ Hits        10019    11058    +1039     
+ Misses       1496     1424      -72     
- Partials      315      341      +26     

☔ View full report in Codecov by Harness.
📢 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.

edithatogo and others added 2 commits July 11, 2026 17:50
Expose Holder.is_input, Simulation.is_input, and Simulation.get_value_state
so screener-style callers can distinguish intentional zeros from omitted
inputs. Calculation defaults are unchanged; this is an opt-in query API
over the existing _user_input_keys tracking.

Closes PolicyEngine#513
Exercise unbound holders, empty user-input keys, and parent-branch
inheritance so Codecov patch coverage clears on PR PolicyEngine#516.

Co-authored-by: Cursor <cursoragent@cursor.com>
@edithatogo edithatogo force-pushed the feature/explicit-input-value-state branch from b5750bb to 1158465 Compare July 11, 2026 07:51
@edithatogo

Copy link
Copy Markdown
Author

Pushed additional unit tests for Holder.is_input edge cases (unbound holder, empty _user_input_keys, parent-branch inheritance) and rebased onto current master. Aimed at the Codecov patch gaps on holder.py.

Delegate current-branch inheritance to Simulation._get_visible_branch_names,
document ETERNITY period caveat, and name planned value-state vocabulary.

Co-authored-by: Cursor <cursoragent@cursor.com>
@edithatogo

Copy link
Copy Markdown
Author

Follow-up to @MaxGhenis review

  1. Holder.is_input now reuses Simulation._get_visible_branch_names() when querying the simulation’s current branch (same walk as exportable inputs). Queries for a different branch_name only check that exact key (avoids the parent_branch coupling you flagged).
  2. ETERNITY caveat documented on the method.
  3. get_value_state docstring names the planned vocabulary (computed / unknown) while keeping today’s explicit / default.

Could you also approve the workflow runs on this fork PR so CI can report? Thanks!

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.

Proposal: preserve missing-input state instead of collapsing it to zero

2 participants