Skip to content
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

Guaranteed Income Supplement + Spousal Allowance #102

Draft
wants to merge 35 commits into
base: master
Choose a base branch
from

Conversation

alex-rand
Copy link
Collaborator

  • make format && make documentation has been run.

New variable

  • Label field added
  • Documentation field added
  • Unit field added
  • Default value field added if relevant
  • Variable name follows conventions
  • Unit test(s) added
  • Integration test(s) added if relevant
  • Issues this PR fixes linked

What's changed

Description of the changes here.

Bug fix

  • Regression test added
  • Regression test passing

What this fixes and how it's fixed

Description of how this fix works goes here. Link any issues this PR fixes.

Copy link
Contributor

@MaxGhenis MaxGhenis left a comment

Choose a reason for hiding this comment

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

Phenomenal, Alex, this is quite the program. No obvious errors here, just some cosmetic suggestions, and in some cases I suggested in one places but it applies to several. Otherwise given the complexity some more comments could help understand it, but I expect I'll learn most from this going through it in the web app and/or with you walking me through it. So I can merge after these with the changelog.

definition_period = YEAR

def formula(person, period, parameters):
p = parameters(period).gov.cra.benefits.gis_spa.gis_exemption
Copy link
Contributor

Choose a reason for hiding this comment

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

unused

Suggested change
p = parameters(period).gov.cra.benefits.gis_spa.gis_exemption

class gis_income(Variable):
value_type = float
entity = Person
label = "Employment income"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
label = "Employment income"
label = "GIS income"

)
& (
gis_base > 0
), # the gis_base > 0 makes sure this person is the eligible one in the couple, since both people in the couple will have the same category.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you split this across lines to stay within 79 characters?

gis_spa_categories = gis_spa_category.possible_values
oas_eligible = person("oas_eligible", period)
p = parameters(period).gov.cra.benefits.gis_spa.topup_cap
return select(
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you simplify this with an | (or) statement, since many of the outcomes are the same? If the default isn't activated, it could be a where.

Same goes for the reduction


return (
(
(2 * p_gis_spa.gis_cap.two_pensioners)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can remove the 2

- name: Not a total reduction
period: 2022
input:
spa_oas_portion: 2000
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
spa_oas_portion: 2000
spa_oas_portion: 2_000

period: 2022
absolute_error_margin: 0.01
input:
gis_spa_category: "SINGLE_WITH_OAS"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
gis_spa_category: "SINGLE_WITH_OAS"
gis_spa_category: SINGLE_WITH_OAS

@@ -0,0 +1,9 @@
description: Reduction rate for the GIS portion of the SPA.
values:
1999-12-31: .25
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
1999-12-31: .25
1999-12-31: 0.25

@@ -0,0 +1,9 @@
description: Reduction rate for the GIS portion of the SPA.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
description: Reduction rate for the GIS portion of the SPA.
description: Canada reduces the GIS portion of the SPA at this rate.

period: year
documentation: Corresponds to GISEMPEXP and GISEMPEXM
label: Percentage of income exempt from inclusion in calculating the GIS reduction.
name: gis_basic_income_exemption_rate
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
name: gis_basic_income_exemption_rate

Copy link
Collaborator

Choose a reason for hiding this comment

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

need changelog

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.

3 participants