Add explicit input / value-state provenance helpers#516
Conversation
MaxGhenis
left a comment
There was a problem hiding this comment.
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_inputreads the_user_input_keysset thatset_inputalready maintains for reform/cache invalidation, so there's no new bookkeeping and no calculation-path cost. - Correctly excludes cache writes.
_user_input_keysis only populated insideset_input's_setpath (gated on_user_input_contexts), soput_in_cachewrites are not treated as inputs — verified by the new test. - The explicit-zero case is exactly right.
set_input(0)reads asexplicit, missing reads asdefault, 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 intest_simulations.py.
Suggestions (non-blocking)
- Reuse
Simulation._get_visible_branch_names()for branch inheritance.Holder.is_inputre-implements the ancestor-plus-defaultwalk by hand, butSimulationalready has the canonical version:_get_visible_branch_names()returns[self.branch_name, …ancestors…, "default"]deduped, and_get_exportable_input_periodsalready 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 usessimulation.parent_branchregardless of thebranch_nameargument, which is only correct whenbranch_name == simulation.branch_name(the only path wired today, viaSimulation.is_input). - ETERNITY periods — worth a check. The reform-reset logic notes that ETERNITY-defined variables canonicalize every period to a single storage key, but
is_inputnormalizes the lookup withperiods.period(period)without that canonicalization. Sois_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. - Document the intended value-state vocabulary.
get_value_statereturns"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 Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
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>
b5750bb to
1158465
Compare
|
Pushed additional unit tests for |
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>
Follow-up to @MaxGhenis review
Could you also approve the workflow runs on this fork PR so CI can report? Thanks! |
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), returningexplicitordefault.Explicit zero remains distinguishable from an omitted/defaulted value. Formula cache writes remain non-inputs. Calculation defaults and formula semantics are unchanged.
Verification
pytest tests/core/test_holders.pypasses locally (25 tests at the recorded revision).make format && make documentationawaits the upstream workflow run.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.