-
Notifications
You must be signed in to change notification settings - Fork 201
Add default parameters to select statements #6286
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add default parameters to select statements #6286
Conversation
- Fixed select statements with dummy True conditions to use default parameter - Added SINGLE as default for filing status select statements - Improved age_group, race, and other demographic variables - Updated WIC, school meals, and elderly/disabled credit calculations - Addresses issue PolicyEngine#1176 for ~10% performance improvement in select operations
- Fixed 7 more state tax files to use SINGLE as default for filing status - Updated federal tax calculation for Social Security taxability - Added defaults to Pell Grant calculations - Continuing work on issue PolicyEngine#1176
- Fixed Medicaid category selection - Updated Hawaii income tax calculation - Total of 19 files updated so far with proper default parameters - Continuing work on issue PolicyEngine#1176
- Created select_filing_status_value() utility in tools/general.py - Refactored 8 state income tax calculations to use the utility - Simplified code by removing repetitive filing status select patterns - Supports additional parameters like right=True for calc methods - Uses SINGLE as default per issue PolicyEngine#3334
- Fixed taxsim_mstat.py to have default=1 for SINGLE filing status - Added defaults to PR earned income credit, Pell Grant calculations - Added safety defaults to NYC and NJ tax calculations - Ensures all select statements handle edge cases appropriately
- Added default=0 to multiple select statements in NJ property tax deduction - Fixed NY inflation refund credit, main income tax, and supplemental tax calculations - Added defaults to OR federal tax liability subtraction and WI standard deduction - Ensures robust handling of edge cases in state tax computations
- Added default=0 to AZ family tax credit eligibility income limits - Refactored CCDF duration of care to use proper default parameter instead of True condition - Ensures consistent pattern usage across all select statements
- Added default=0 to CCDF market rate calculation - Replaced True conditions with proper default parameters in taxsim_state and fsla_overtime_occupation_exemption_category - Ensures consistent select statement pattern usage
…ults - Refactored 13 state tax files to use select_filing_status_value utility - Added default parameters to 7 files that couldn't use the utility - Fixed MT, VT, GA, IA, CT, WV, NC, NM tax calculations - Ensures consistent handling of filing status across state tax code
- Refactored HI, ID, LA, MD, ME, WI tax calculations to use select_filing_status_value - Added default parameters where utility couldn't be used (IL exemptions) - Ensures consistent filing status handling across all state tax code
- Created YAML test verifying MD tax calculations use the utility correctly - Tests single, joint, and head of household filing statuses - Verifies that different filing statuses get correct tax rates
- Fixed NJ main income tax, retirement exclusion, and property tax deduction - Refactored MT individual income tax to use utility - Updated NM medical care deduction and blind/aged exemption - Fixed ME sales tax fairness credit with custom parameter handling - Updated MD personal exemption and senior tax credit with parameter mappings - All files now use select_filing_status_value utility for cleaner code
- Add default=CCDFAgeGroup.SCHOOL_AGE for teenagers (age >= 13) in ccdf_age_group.py - Refactor or_federal_tax_liability_subtraction.py to use select_filing_status_value utility - NY/NYC tax files were already refactored in previous commits - All tests pass for modified files
policyengine_us/variables/gov/local/ny/nyc/tax/income/credits/household/nyc_household_credit.py
Show resolved
Hide resolved
… enums - Handle cases where state filing status enums don't have SURVIVING_SPOUSE - Use hasattr to check for enum value existence before accessing - Remove problematic integration test that incorrectly used Microsimulation
The test was missing household_count_people and household_weight, causing a ZeroDivisionError when calculating weighted deciles
Since pell_grant_uses_efc and pell_grant_uses_sai are mutually exclusive (based on the same enum that can only be EFC or SAI), we can simplify by: - Removing the pell_grant_uses_sai variable - Using ~uses_efc where we previously used uses_sai This reduces code duplication and makes the logic clearer.
Replace pell_grant_calculation_method enum and pell_grant_uses_efc with a single pell_grant_uses_sai boolean variable that returns True from 2024 onwards (when SAI is used) and False before 2024 (when EFC is used). This simplifies the code by removing unnecessary complexity while maintaining the same functionality. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Convert select statements that use a condition and its negation to simpler where statements for better readability and performance. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we confirm that the unit test passes? Don't see the execution in the testing logs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not blocking but it would be nice to see unit tests with the same md_taxable_income amount but different md_income_tax_before_credits based on household composition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a unit test, just to sanity test years such as 2022 and 2025
| CCDFAgeGroup.PRESCHOOLER, | ||
| CCDFAgeGroup.SCHOOL_AGE, | ||
| ], | ||
| default=CCDFAgeGroup.SCHOOL_AGE, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it not default to infant to match the default value on line 14?
| duration_of_care == durations_of_care.HOURLY, | ||
| ], | ||
| [1, days_per_week, days_per_week, hours_per_week], | ||
| default=0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, should this not default to 1 if the default value of ccdf_duration_of_care is weekly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also haven't unit tested this -- can we add while in here?
| p.separate.calc(income), | ||
| ], | ||
| ) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is missing a surviving_spouse parameter file, need to add
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting that it was not breaking before, maybe was just not caught yet, we should add a unit test
| @@ -1,4 +1,5 @@ | |||
| from policyengine_us.model_api import * | |||
| from policyengine_us.tools.general import select_filing_status_value | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this import necessary? Not present in most adjusted files
| Race.BLACK, | ||
| Race.OTHER, | ||
| ], | ||
| default=Race.OTHER, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we adjust the default value on line 14 to "OTHER" to match the output?
|
Closing in favor of a cleaner reimplementation - the PR grew too complex. |
|
This PR has been sharded into separate PRs:
The remaining ~40 files using the filing status utility can be migrated in follow-up PRs after #7243 is reviewed and merged. |
Summary
This PR addresses issue #1176 by adding
defaultparameters toselectstatements throughout the codebase. This provides ~10% performance improvement and makes the code more explicit about default values.Changes
Trueconditions withdefaultparameter in select statementsSINGLEas default for filing status select statements (per issue Replace comprehensiveselectstatement byfiling_statuswithdefaultofSINGLE#3334)Test plan
Notes
This is a partial implementation focusing on the clearest cases. There are still ~110+ select statements without defaults that would benefit from individual review to determine appropriate default values.
Fixes #1176
Related to #3334