From 00fe19e5a28d01fc11dde45cbbcb606da85dd9c3 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 23:31:43 +0000 Subject: [PATCH 1/3] Fix docs helper import and tighten docs and test hygiene The documentation helper imported colours Core never exported, so the CTC, ACP and Lifeline notebooks failed on import. The PIA-ratio guide was linked from the calculator page but missing from the table of contents. Comments in the pinned-system test described interpolation metadata Core no longer writes. The calculator reference test checked only direct children, so a calculator-only leaf under a kept node passed, and a stale NEW_PARAMETER_NODES path went unnoticed because apply skips it silently. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LwQq6H3e3QDf7E1R8JgS82 --- changelog.d/docs-test-hygiene.fixed.md | 1 + docs/_toc.yml | 1 + .../tests/test_pinned_tbs_sharing.py | 7 +- .../unit/test_ss_calculator_differential.py | 193 +++++++++++++++--- policyengine_us/tools/documentation.py | 24 ++- 5 files changed, 183 insertions(+), 43 deletions(-) create mode 100644 changelog.d/docs-test-hygiene.fixed.md diff --git a/changelog.d/docs-test-hygiene.fixed.md b/changelog.d/docs-test-hygiene.fixed.md new file mode 100644 index 00000000000..80d7d00ec06 --- /dev/null +++ b/changelog.d/docs-test-hygiene.fixed.md @@ -0,0 +1 @@ +Fix the documentation helper's chart import for current Core, list the PIA-ratio guide in the docs table of contents, update stale interpolation comments in the pinned-system test, and make the calculator reference test classify every leaf under kept parameter nodes and flag missing calculator-only paths. diff --git a/docs/_toc.yml b/docs/_toc.yml index aef7dfab381..865cb67960a 100644 --- a/docs/_toc.yml +++ b/docs/_toc.yml @@ -68,6 +68,7 @@ parts: - file: usage/social-security-calculator sections: - file: usage/social-security/RULINGS + - file: usage/social-security-pia-ratio - file: usage/social-security-oasdi-accounting - file: usage/speed - caption: Examples diff --git a/policyengine_us/tests/test_pinned_tbs_sharing.py b/policyengine_us/tests/test_pinned_tbs_sharing.py index 84a0c941e8b..49fa3a0855a 100644 --- a/policyengine_us/tests/test_pinned_tbs_sharing.py +++ b/policyengine_us/tests/test_pinned_tbs_sharing.py @@ -183,7 +183,9 @@ def change_small_tree(tree, change, index, value): ) parameter.values_list.sort(key=lambda item: item.instant_str, reverse=True) elif change == "change metadata": - # What interpolation does to a parameter's metadata. + # What interpolation did to a parameter's metadata in core before + # 3.34.1, which recorded it there; core now sets the parameter's + # `interpolated` attribute instead. parameter.metadata.setdefault("interpolation", {})["completed"] = value elif f"added_{value}" not in tree.tax.children: added = Parameter(f"added_{value}", {"values": {AT: value}}) @@ -302,7 +304,8 @@ def test_a_copy_of_the_nodes_reads_what_a_clone_reads(changes, later_changes): def test_each_tree_interpolates_for_itself(): - """Core records in a parameter's metadata that it interpolated the parameter. + """Core records on each parameter that it interpolated it, and core before + 3.34.1 recorded it in the parameter's `interpolation` metadata. A record shared with the source would stop the copy interpolating at all. """ diff --git a/policyengine_us/tests/unit/test_ss_calculator_differential.py b/policyengine_us/tests/unit/test_ss_calculator_differential.py index d59466ec760..b5c15d881ef 100644 --- a/policyengine_us/tests/unit/test_ss_calculator_differential.py +++ b/policyengine_us/tests/unit/test_ss_calculator_differential.py @@ -15,6 +15,7 @@ import numpy as np import pytest +from policyengine_core.parameters import ParameterNode from policyengine_core.reforms import Reform from policyengine_us import Simulation @@ -31,26 +32,110 @@ "gov.ssa.social_security.pia.bend_points", "gov.ssa.social_security.cola", ) -# Contributed namespaces whose every child node must be classified: either -# calculator-only (listed in NEW_PARAMETER_NODES, so the reference drops it) -# or kept in the reference (listed below with the code outside the household -# calculator that reads it). +# Contributed namespaces whose every leaf must be classified: either +# calculator-only (at or under a path in NEW_PARAMETER_NODES, so the reference +# drops it) or kept in the reference (listed below under its kept node, whose +# comment names the code outside the household calculator that reads it). CLASSIFIED_CONTRIB_NAMESPACES = ("gov.contrib.congress.s770", "gov.contrib.ssa") -REFERENCE_PARAMETER_NODES = ( +# Each kept node maps to every leaf beneath it, so a calculator-only leaf added +# under a kept node fails the classification test until someone lists it here +# or in NEW_PARAMETER_NODES. +REFERENCE_PARAMETER_NODES = { # tools/social_security/population.py: reported-benefit PIA ratios. - "gov.contrib.congress.s770.section_2", + "gov.contrib.congress.s770.section_2": ( + "in_effect", + "recompute_pre2026_death_before_eligibility", + ), # reforms/congress/s770/section_8.py reads it at simulation start; the # s770 tax variables read it in formulas. - "gov.contrib.congress.s770.section_8", + "gov.contrib.congress.s770.section_8": ( + "active_business.deny_nol", + "active_business.in_effect", + "active_business.oasdi_share", + "active_business.rate", + "active_business.start_year", + "nii_addon.in_effect", + "nii_addon.oasdi_share", + "nii_addon.rate", + "nii_addon.start_year", + "transfer.bill_share", + "transfer.mode", + "transfer.oasdi_rate", + "transfer.start_year", + ), # The social_security_*_payable_factor and across-the-board variables, # the COLA means-test reform and the reported-benefit PIA ratios. - "gov.contrib.ssa.benefit_levers", + "gov.contrib.ssa.benefit_levers": ( + "across_the_board.rate", + "across_the_board.start_year", + "cola.alternative_rate", + "cola.effective_month", + "cola.effective_year", + "cola.end_year", + "cola.means_test.below_threshold_cola_index", + "cola.means_test.effective_month", + "cola.means_test.effective_year", + "cola.means_test.enabled", + "cola.means_test.filing_groups.HEAD_OF_HOUSEHOLD", + "cola.means_test.filing_groups.JOINT", + "cola.means_test.filing_groups.SEPARATE", + "cola.means_test.filing_groups.SINGLE", + "cola.means_test.filing_groups.SURVIVING_SPOUSE", + "cola.means_test.first_eligible_year", + "cola.means_test.income_sensitivity", + "cola.means_test.pre_receipt_cola_index", + "cola.means_test.scope", + "cola.means_test.thresholds.base_year", + "cola.means_test.thresholds.index_lag", + "cola.means_test.thresholds.joint", + "cola.means_test.thresholds.rounding", + "cola.means_test.thresholds.single", + "cola.pp_change", + "cola.start_year", + "cola.withholding_enabled", + "payable.oasi_only", + "payable.share", + "pia_ratio.annual_rounding_bound", + "pia_ratio.claiming_scenario", + "pia_ratio.disability_eligibility_proxy", + ), # reforms/ssa/owners_draw.py and s770_active_business_income; system.py # writes current_law_cap into it at every system build. - "gov.contrib.ssa.owners_draw", + "gov.contrib.ssa.owners_draw": ( + "benefit_credit", + "current_law_cap", + "half_seca_deduction", + "in_effect", + "include_hi", + "material_participation_only", + "net_earnings_factor", + "phase_in_share", + "phase_in_years", + "room_cap_basis", + "start_year", + ), # tools/social_security/population.py: reported-benefit PIA ratios. - "gov.contrib.ssa.pia", -) + "gov.contrib.ssa.pia": ( + "bend_point_multipliers.first", + "bend_point_multipliers.second", + "effective_year", + "factors.first", + "factors.second", + "factors.start_year", + "factors.third", + "full_price_indexing.in_effect", + "full_price_indexing.real_wage_scale", + "full_price_indexing.start_year", + "in_effect", + "progressive_price_indexing.bend_point_fraction", + "progressive_price_indexing.in_effect", + "progressive_price_indexing.maximum_aime", + "progressive_price_indexing.maximum_pia_target", + "progressive_price_indexing.start_year", + "progressive_price_indexing.threshold", + "recompute_existing", + ), +} YEAR = 2026 @@ -108,44 +193,86 @@ def apply(self): def _node(root, path): + """Return the node at ``path``, or None if any part of it is missing.""" node = root for part in path.split("."): - node = node.children[part] + children = getattr(node, "children", None) + if children is None or part not in children: + return None + node = children[part] return node -def test_every_contributed_calculator_namespace_child_is_classified(): - """A new node under these namespaces must be calculator-only or kept. +def _leaves(node, prefix): + """Yield the path of every leaf parameter under ``node``, at any depth.""" + for name, child in node.children.items(): + path = f"{prefix}.{name}" + if isinstance(child, ParameterNode): + yield from _leaves(child, path) + else: + yield path + + +def _under(path, ancestor): + return path == ancestor or path.startswith(f"{ancestor}.") - If an unlisted calculator-only node stayed in the reference, a legacy + +def test_every_contributed_calculator_namespace_leaf_is_classified(): + """A new leaf under these namespaces must be calculator-only or kept. + + If an unlisted calculator-only leaf stayed in the reference, a legacy formula that read it by accident would compute the same in both models, - and the differential below could not detect the dependency. + and the differential below could not detect the dependency. The walk goes + to every depth, so a calculator-only leaf added under a kept node fails + here too. Every listed path must also exist, because + ``WithoutHouseholdCalculator.apply`` silently skips a missing one. """ parameters = Simulation.default_tax_benefit_system_instance.parameters - classified = NEW_PARAMETER_NODES + REFERENCE_PARAMETER_NODES + classified = NEW_PARAMETER_NODES + tuple(REFERENCE_PARAMETER_NODES) problems = [ f"{path} is listed {classified.count(path)} times" for path in sorted(set(classified)) if classified.count(path) > 1 ] - children = set() - for namespace in CLASSIFIED_CONTRIB_NAMESPACES: - for name in _node(parameters, namespace).children: - path = f"{namespace}.{name}" - children.add(path) - if path not in classified: - problems.append( - f"{path} is unclassified: add it to NEW_PARAMETER_NODES if " - "only the household calculator reads it, or to " - "REFERENCE_PARAMETER_NODES with a comment naming the " - "legacy formula, reform start-up or system build that " - "reads it" - ) problems += [ - f"{path} is listed in REFERENCE_PARAMETER_NODES but does not exist" - for path in REFERENCE_PARAMETER_NODES - if path not in children + f"{path} is listed in NEW_PARAMETER_NODES but does not exist" + for path in NEW_PARAMETER_NODES + if _node(parameters, path) is None ] + kept_leaves = set() + for kept, leaves in REFERENCE_PARAMETER_NODES.items(): + if _node(parameters, kept) is None: + problems.append( + f"{kept} is listed in REFERENCE_PARAMETER_NODES but does not exist" + ) + for leaf in leaves: + path = f"{kept}.{leaf}" + if path in kept_leaves: + problems.append(f"{path} is listed more than once") + kept_leaves.add(path) + if _node(parameters, path) is None: + problems.append( + f"{path} is listed under {kept} in " + "REFERENCE_PARAMETER_NODES but does not exist" + ) + for namespace in CLASSIFIED_CONTRIB_NAMESPACES: + for path in _leaves(_node(parameters, namespace), namespace): + if any(_under(path, new) for new in NEW_PARAMETER_NODES): + continue + if path in kept_leaves: + continue + kept = [node for node in REFERENCE_PARAMETER_NODES if _under(path, node)] + where = ( + f"under {kept[0]} in REFERENCE_PARAMETER_NODES if the code " + "named in that node's comment reads it" + if kept + else "to REFERENCE_PARAMETER_NODES with a comment naming the " + "legacy formula, reform start-up or system build that reads it" + ) + problems.append( + f"{path} is unclassified: add it to NEW_PARAMETER_NODES if " + f"only the household calculator reads it, or list it {where}" + ) assert not problems, "\n".join(problems) diff --git a/policyengine_us/tools/documentation.py b/policyengine_us/tools/documentation.py index 6f2b9ba463e..25d4ecbd24b 100644 --- a/policyengine_us/tools/documentation.py +++ b/policyengine_us/tools/documentation.py @@ -6,13 +6,16 @@ warnings.filterwarnings("ignore") from policyengine_core.enums import Enum -from policyengine_core.charts import ( - LIGHTER_BLUE, - LIGHT_BLUE, - BLUE, - DARK_BLUE, - format_fig, - display_fig, +from policyengine_core.charts import format_fig, display_fig + +# Core's chart palette has no LIGHTER_BLUE, LIGHT_BLUE or DARK_BLUE, and +# the package exports only BLUE and the three-shade BLUE_COLOUR_SCALE, so +# take four blues, light to dark, from the module that defines them. +from policyengine_core.charts.formatting import ( + BLUE_LIGHT, + BLUE_PRIMARY, + BLUE_PRESSED, + DARKEST_BLUE, ) from policyengine_us.system import system from policyengine_us import Simulation @@ -117,7 +120,12 @@ def variation_chart( color="Children", facet_col="Adults", facet_col_spacing=0.1, - color_discrete_sequence=[LIGHTER_BLUE, LIGHT_BLUE, BLUE, DARK_BLUE], + color_discrete_sequence=[ + BLUE_LIGHT, + BLUE_PRIMARY, + BLUE_PRESSED, + DARKEST_BLUE, + ], ) fig.update_xaxes( tickformat="$,.0f", From f1ebe9d2d853561b64ada8b1b5932b58639fecf7 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Mon, 2 Nov 2026 18:59:14 -0500 Subject: [PATCH 2/3] Give the source-conflict guard's texts CONTRIBUTING's order #10588 made the ruling PR record a candidate, with the register entry and the marker after it merges. The failure message and the KNOWN_UNMARKED comment still said to register and mark in one PR, and the docstring said verify checks that cites_entry is true, where it checks that the claim matches the file. Co-Authored-By: Claude Opus 5.5 --- .../tests/code_health/test_source_conflicts.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/policyengine_us/tests/code_health/test_source_conflicts.py b/policyengine_us/tests/code_health/test_source_conflicts.py index 5cb01c89cf6..6d4c3c8660a 100644 --- a/policyengine_us/tests/code_health/test_source_conflicts.py +++ b/policyengine_us/tests/code_health/test_source_conflicts.py @@ -47,7 +47,8 @@ These are structural checks: they tie markers, the register and the backlog together; they do not judge whether code follows a statute. rulespec-us's own ``verify`` checks the other half at pinned commits: that each listed file and -test exists and that ``cites_entry`` is true. +test exists and that each ``cites_entry`` claim matches whether the file names +the entry. """ from __future__ import annotations @@ -114,9 +115,10 @@ ) # Files with conflict-sounding comments and no marker, and how many such -# lines each has. A file may only go down: register and mark the conflict, -# or reword a comment that is not one, then lower or delete its count here. -# A new file or a higher count fails. +# lines each has. A new file or a higher count fails, so reword a comment that +# describes no conflict. A real conflict stays: record it as a candidate and add +# or raise its line here in the same PR, and the line goes when the marker lands +# (CONTRIBUTING.md, "Source conflicts"). A lower count must be recorded here too. KNOWN_UNMARKED: dict[str, int] = { "policyengine_us/parameters/gov/irs/ald/charitable_contributions/cap.yaml": 1, "policyengine_us/parameters/gov/irs/credits/residential_clean_energy/fuel_cell_cap_per_kw.yaml": 1, @@ -443,8 +445,10 @@ def test_conflict_sounding_comments_do_not_grow(): assert not grown, ( "These files gained comments that read like a conflict between sources, with no " "`# Source conflict :` marker. If a statute and a form, instructions, manual or " - "agency practice disagree, register it (rulespec-us known-source-conflicts.yaml) and " - f"mark the deciding line; otherwise reword the comment. Files and counts: {grown}" + "agency practice disagree, record it in source_conflicts/candidates.yaml and add or raise " + "the file's KNOWN_UNMARKED line in this PR; the register entry and the marker come after " + 'it merges (CONTRIBUTING.md, "Source conflicts"). ' + f"Otherwise reword the comment. Files and counts: {grown}" ) assert not shrunk, ( "These files now have fewer unmarked conflict-sounding comments; lower or delete " From 20a315a252e35abe3de9f6575b68781e75dbb680 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Mon, 2 Nov 2026 19:42:25 -0500 Subject: [PATCH 3/3] List the Social Security lever pages in the table of contents The calculator page links them, and nothing in the navigation led to them. The review built this table of contents with CI's toolchain, and it exits 0. Also let the source-conflict guard's message cover a conflict the pinned register already lists, since test_candidates_are_not_yet_registered fails a second candidate for one. Co-Authored-By: Claude Opus 5.5 --- docs/_toc.yml | 5 +++++ policyengine_us/tests/code_health/test_source_conflicts.py | 5 +++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/docs/_toc.yml b/docs/_toc.yml index 865cb67960a..0e526a13322 100644 --- a/docs/_toc.yml +++ b/docs/_toc.yml @@ -69,7 +69,12 @@ parts: sections: - file: usage/social-security/RULINGS - file: usage/social-security-pia-ratio + - file: social-security-benefit-levers + sections: + - file: social-security-cola-means-test - file: usage/social-security-oasdi-accounting + sections: + - file: usage/social-security-scoring-preflight - file: usage/speed - caption: Examples chapters: diff --git a/policyengine_us/tests/code_health/test_source_conflicts.py b/policyengine_us/tests/code_health/test_source_conflicts.py index 6d4c3c8660a..21f6c365198 100644 --- a/policyengine_us/tests/code_health/test_source_conflicts.py +++ b/policyengine_us/tests/code_health/test_source_conflicts.py @@ -445,8 +445,9 @@ def test_conflict_sounding_comments_do_not_grow(): assert not grown, ( "These files gained comments that read like a conflict between sources, with no " "`# Source conflict :` marker. If a statute and a form, instructions, manual or " - "agency practice disagree, record it in source_conflicts/candidates.yaml and add or raise " - "the file's KNOWN_UNMARKED line in this PR; the register entry and the marker come after " + "agency practice disagree, record it in source_conflicts/candidates.yaml (unless the " + "pinned register already lists it) and add or raise the file's KNOWN_UNMARKED line in " + "this PR; the register entry and the marker come after " 'it merges (CONTRIBUTING.md, "Source conflicts"). ' f"Otherwise reword the comment. Files and counts: {grown}" )