-
Notifications
You must be signed in to change notification settings - Fork 23
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
Refurbishment #37
Refurbishment #37
Conversation
Also, one final thing I didn't mention is adding two modelling-related reforms, benefits_as_reported (which turns off benefits simulation) and just_data (which turns off all simulation). Idea being if we're just abolishing benefits, we should probably use benefits_as_reported as there's no reason to simulate benefits as that can only introduce error. |
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.
Few minor comments on the first ~third of files I scrolled through, mostly around making descriptions consistent and more comprehensive by themselves (e.g. not abbreviating program names in them), curious what you think.
@@ -0,0 +1,8 @@ | |||
description: Threshold in pounds per week from earnings per week, above which the contributory JSA amount is reduced equally. |
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 this read "housing benefit" rather than JSA? Similarly a few other descriptions seem to not match the folder structure.
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.
Yep, a lot of them are copied, I'll see which need to be corrected
@@ -0,0 +1,8 @@ | |||
description: Threshold in pounds per week from income for a lone parent, above which the Income Support amount is reduced equally. |
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.
looks to duplicate income_disregard_lone_parent.yml
@@ -1,6 +1,6 @@ | |||
description: Threshold in pounds per week from income for a lone parent, above which the Income Support amount is reduced equally. |
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.
For consistency, could you rename this (and other *_lone.yml
files) to income_disregard_lone_parent.yml
? Looks to be how others are named.
@@ -0,0 +1,6 @@ | |||
description: Default hours requirement |
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.
For consistency please add a period here (and other descriptions), or remove from others.
@@ -0,0 +1,7 @@ | |||
description: LHA maximum rate for accommodation in Maidstone, Kent |
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 LHA only for Maidstone, Kent? Is there an appropriate place to document the benefit at a high level, e.g. benefits/LHA/README.md
? If nothing else to unabbreviate.
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're only using the rates for Maidstone, Kent, following TaxBEN's example. I'm also thinking we should make separate documentation for the whole model?
@@ -1,6 +1,6 @@ | |||
description: Weekly income_support applicable amount for lone parents aged over 18. |
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.
description: Weekly income_support applicable amount for lone parents aged over 18. | |
description: Weekly Income Support applicable amount for lone parents aged over 18. |
@@ -0,0 +1,7 @@ | |||
description: Earnings disregard for means testing Universal Credit per month |
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.
Are earnings disregards equivalent to personal allowances (e.g. as the housing benefit refers to)?
@@ -0,0 +1,6 @@ | |||
description: Basic element of WTC |
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.
description: Basic element of WTC | |
description: Basic element of Working Tax Credit, upon which other elements stack. |
@@ -0,0 +1,9 @@ | |||
description: Yearly income threshold after which child tax credits are reduced for benefit units only claiming CTC and not WTC. |
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.
Similar to above, would this be describable as an earnings disregard or personal allowance?
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.
I think an earnings disregard is probably clearest, though I have seen either used.
@@ -0,0 +1,6 @@ | |||
description: Couple with children hours requirement |
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.
description: Couple with children hours requirement | |
description: Minimum weekly hours of work for couples with children to be eligible for the Working Tax Credit |
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 summed between both members of the couple?
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.
Yep, but there's an additional requirement for at least one member to work at least 16 hours a week.
@MaxGhenis I agree, the parameters definitely need a bit more verbosity and description in them, and also there's probably some big code minimisation gains to be had from using this mapping feature. Most of these parameter descriptions haven't changed in the PR, I wonder if it's worth making the parameter descriptiveness improvements in their own PR, or should we add it to this one? |
Setting aside for another PR makes sense, just filed #39 to be addressed whenever. |
Merging, with parametric details to be addressed in a separate PR |
Model refurbishment
This is quite a substantial PR, apologies for any difficulties in comparisons. It addresses and fixes a few underlying problems/imperfections we had:
Parallel PRs in frs and a fork of openfisca-core are here and here respectively.
This is a pretty big PR, but I started with implementing the time periods, which meant everything else had to be adjusted, and wanted to make sure all functionality was preserved before merging, so rewrote some parts while they were going to be adjusted anyway.
Time periods
Previously, we had mostly ignored OpenFisca standard time period usage because it didn't have the WEEK period, and most benefits are specified in weekly amounts. This was always going to need addressing at some point, so I implemented the WEEK period in openfisca-core (meaning we are now diverging from the standard openfisca-core into my fork of it if it isn't in line with their policy), so now benefits are calculated in each week of the year, and taxes at the yearly level, etc. It seems this is the most "correct" way of doing things. It also means that phase-in reforms are a) actually possible and b) not very much work. By loading in multiple years of the FRS, for example, we could specify a parametric reform with different parameters for each year, and calculate the time-series effects.
Reorganisation
Benefits now all have their own file, and generally things are decomposed a bit more - hopefully, this makes the repo more readable.
Tests, accuracy improvements
A few missing features were added after adding more tests: the LHA rate caps, the benefit cap and disability estimation follows EUROMOD specification. More tests were added using the new time periods structure. In general, things require more specification now we are adding time periods into it: previously, we used ETERNITY for everything, which meant we passed in a redundant period in every call that only sometimes specified the period of parameters used, and never the actual data.
Model API improvements
Wrapper class, Simulation
I added a wrapper class for OpenFisca-UK with a few time-saving functions. Firstly, because we're now using time periods instead of not at all, where we previously just found the e.g. reported Pension Credit per week and passed it in with ETERNITY, the load_frs function now finds the pension_credit_reported variable in the input CSV file, looks up the time period of pension credit in the model, finds it has period WEEK, and loads it into all the WEEKs in the input year (2020).
Mapping entity variables
A very time-consuming issue previously was calculating a group variable for its members outside of OpenFisca-UK internal formulas. For example, if we wanted to calculate the household income of each person in the simulation, instead of each household. This is now just a matter of saying so in an argument - variables are mapped downwards onto their person members. Household to benefit unit mapping isn't included, but I have yet to need this.
MTR improvements
We used to calculate MTRs by including a hacky variable on the internal formulas and including it wherever earnings were used. Now, we just generate an alternate simulation with earnings increased by £100 per year, and calculate the percentage of an individual's increase which doesn't appear in the person's household's net income increase. This now takes a bit longer, but it's more accurate: we now add £100 to the earnings to all the "household person 1"s of the population, calculate their MTR, repeat for persons 2 through 9 (the max household number). Almost definitely some performance increases here if it's critical; I haven't optimised it. More importantly, we can now pass back a breakdown of the MTRs thanks to the variable mapping - not just the MTR of e.g. 0.73, but also an explanation of where it comes from (e.g. Income tax 20%, NI 12%, WTC 41%).