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

Remove OpenFisca-UK fork dependency + other changes #139

Merged
merged 26 commits into from
Jul 5, 2021
Merged

Remove OpenFisca-UK fork dependency + other changes #139

merged 26 commits into from
Jul 5, 2021

Conversation

nikhilwoodruff
Copy link
Collaborator

Moderately-sized update to OpenFisca-UK (with a version change from 0.3.0 to 0.4.0):

Back on the official OpenFisca-Core

This PR removes the dependency for openfisca-uk to run off the fork of OpenFisca-Core. What did it cost? All variables are now yearly. There are pros and cons to this, but after considering them, I think the pros really outweigh with very little contest:

  • Cons:
    • The formulas don't look as nice
    • Years with 53 weeks will be out by 53/52. But we could easily account for this and I'm pretty sure other calculators do this
    • Not much else
  • Pros:
    • Better parity with other openfisca models and openfisca itself
    • Model runtime is down by 50% (!)
    • Much easier debugging of failed tests and easier tracing of variables, because the individual week calculations overcomplicated a lot of outputs
    • Easier installation

Reform tests and postcode handling

In preparation for using OpenFisca-UK as a tax-benefit web calculator, I added functionality to take a household's postcode as input (calculating LHA/etc downstream). I've also added the first reform test as a demonstration.

Code variable naming convention testing

I added a test that checks variable names against their return types, using the convention that was mostly (but inconsistently) followed:

  • num_ => int
  • is_ => bool
  • anything else => (bool, Enum, float)

@MaxGhenis

Copy link
Collaborator

@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.

This is great, added a few questions.

- name: Test baseline systems
run: pytest tests/code

- name: Test baseline systems
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this have a different name from above?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, changed

@@ -0,0 +1,380 @@
,LAD20CD,LAD20NM
Copy link
Collaborator

Choose a reason for hiding this comment

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

is the index needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, removed

@@ -0,0 +1,380 @@
,LAD20CD,LAD20NM
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you add a README.md to this folder specifying the data source?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, added

@@ -341,9 +341,6 @@ def apply(self):
"Unable to compute derivative - target variable must be from a group of or the same as the source variable"
)

def mtr(self, *args, **kwargs):
Copy link
Collaborator

Choose a reason for hiding this comment

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

So users should now use deriv to calculate MTRs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, and this removes ambiguity. The current documentation doesn't detail this, I've added an issue at #141 .

reference = "Child Tax Credit Regulations 2002 s. 8"

def formula(person, period, parameters):
benefit = parameters(period).benefit
threshold_safety_gap = 10
THRESHOLD_SAFETY_GAP = 10 * WEEKS_IN_YEAR
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this? should 10 be a policy parameter?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should really be documented, but from memory it's because there are some small variations around the disability benefits paid, probably because of weeklyisation/irregular payments. But where we're imputing the basic/higher levels of benefits based on their amounts, we err on the side of being generous (£10/week is less than the difference between the levels).

add(benunit, period, ["working_tax_credit"], options=[DIVIDE])
+ add(benunit, period, ["ESA_income"], options=[ADD])
+ aggr(benunit, period, QUAL_PERSONAL_BENEFITS, options=[ADD])
add(benunit, period, ["working_tax_credit"])
Copy link
Collaborator

Choose a reason for hiding this comment

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

How does it know to divide this one and add the next one? In general since this PR removes a bunch of options= args could you share how this works?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To recap here, it's because by making everything yearly, we've shifted from doing the adding/dividing time periods in this specific variables to doing the same operations within working_tax_credit, ESA_income and the others.

@@ -1,12 +0,0 @@
- name: Poverty gap for single person
Copy link
Collaborator

Choose a reason for hiding this comment

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

why delete?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hasn't been deleted, just moved (as has disability.yaml).

@nikhilwoodruff
Copy link
Collaborator Author

@jpycroft, @jdebacker, @rickecon FYI this also relaxes the requirements from == to >=.

@nikhilwoodruff nikhilwoodruff merged commit c70220d into PolicyEngine:master Jul 5, 2021
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.

2 participants