-
Notifications
You must be signed in to change notification settings - Fork 19
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
[WIP] Add organ donor compensation #16
base: master
Are you sure you want to change the base?
Conversation
Basic scenario working
* tests: Changed to 52 week calculation
Make reference actually make sense
Update live_organ_donor_compensation.py
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.
Excellent! That seems almost mergeable as such, congratulations for such a clean and tested implementation! I was really impressed with the organiccoders.allengeer.com
URIs as references, with proper semantics 👏 Do you expect these URLs to be stable over time?
Since I opened this PR I cannot approve it. There are a couple comments I made you might want to address, and we'll need to update the changelog, but other than that this seems good to go!
Are you expecting to add some major functionality in there or are you be happy with the current state of things? 😃
value_type = float | ||
entity = Person | ||
definition_period = YEAR # TODO - determine whether we need to get WEEK to work | ||
label = u"The amount deducted as a kiwisaver contribution" |
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.
Adding a legislative link as a
reference
property to these variables would help a lot in double-checking the implementation 🙂
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.
Fixed
# value_type = float | ||
# entity = Person | ||
# definition_period = YEAR # TODO - determine whether we need to get WEEK to work | ||
# label = u"The salary earned by a person before tax" |
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.
Version control (Git) can help us recover code if we need it, but commented-out code usually becomes a problem over time as we cannot know if it is still up-to-date or not… Could we maybe remove these parts from the model? 🤔
class weekly_compensation_before_tax(Variable): | ||
value_type = float | ||
entity = Person | ||
definition_period = YEAR # TODO - determine whether we need to get WEEK to work |
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 comment still relevant? It seems this code works well with YEAR
, even though it would be more proper with WEEK
.
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.
In the current scheme, ETERNITY may be better since the value doesn't correspond to a specific MONTH or YEAR.
I think the value actually corresponds to a particular point in time corresponding to the date of the latest pay period that has been used in the calculation.
CircleCI is still waiting because building forks was not activated yet. I just switched it on, any commit on this branch will trigger a build 🙂 |
* 'master' of github.com:OrganicCoders/openfisca-aotearoa: Update live_organ_donor_compensation.py
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.
Please do not merge yet. I think there's more discussion to be had about the implementation.
The organiccoders.allengeer.com
URLs are impermanent, with the server likely to be shut down soon. We need to consider where these might transition to?
Also it is far from complete in terms of the legislation. It is currently missing shareholder employee and overseas earners, and could do with a review by someone more familiar with the legislation if it is to be used in earnest.
Ideally we would have modules such as Income Tax, Student Loans, KiwiSaver etc that we could call to make this more complete.
We test drove the implementation so it was automatically well tested :)
class weekly_compensation_before_tax(Variable): | ||
value_type = float | ||
entity = Person | ||
definition_period = YEAR # TODO - determine whether we need to get WEEK to work |
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.
In the current scheme, ETERNITY may be better since the value doesn't correspond to a specific MONTH or YEAR.
I think the value actually corresponds to a particular point in time corresponding to the date of the latest pay period that has been used in the calculation.
value_type = float | ||
entity = Person | ||
definition_period = YEAR # TODO - determine whether we need to get WEEK to work | ||
label = u"The amount deducted as a kiwisaver contribution" |
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.
Fixed
return round(employee_weekly_earnings + self_employed_weekly_earnings, 2) | ||
|
||
class kiwisaver_employee_deduction(Variable): | ||
value_type = float |
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 method might be better in a KiwiSaver specific module, so that it picks up any changes to KiwiSaver.
It's also largely copy and paste from above. It should utilise the method from above to calculate the basic compensation and then calculate KiwiSaver on that amount.
It is also missing concepts such as KiwiSaver holiday period.
value_type = int | ||
default_value = 0 | ||
entity = Person | ||
label = u"Kiwisaver employee deduction percentage " |
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.
Extra space in end of string here
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.
Fixed, thanks
Thanks a lot for all these changes already! 👏
The best would probably be legislation.govt.nz 🙂 If there is no better reference, this is still much better than nothing!
This will always be the case. We never know what we don't know from the legislation. This is already the most accurate model available 🙂 It is fine to proceed with this PR and add more details in further ones. |
re the URLs as persistent ids, I think that given we couldn't execute them as proper URIs and have no one to manage these persistent ids, that we don't have much choice than to point to the legislation. The problem with this however, is that this URL will change over time, and so it is not a good long term solution. It points to a piece of work that in Aus our working group does for free, which perhaps doesn't exist here- that is, management of persistent ids. in Aus, the Linked Data working group have linked.data.gov.au and that runs alongside data.gov.au, giving us a home from which to manage this stuff. The info and link to the Persistent Id service is listed on the showcase page here. Something for the team to work through perhaps. |
Thanks @BrigetteM. I can see how linked.data.gov.au would help with this. Hope you've shown that to Pia and co. |
Added definitions of 'number of weeks' and 'earnings as an employee'
I've updated this PR with:
Note that if we go with this and this pull request is actioned, we will need to update the repository name in the reference URLs. There could be a way to resolve this in future using something like python-feedparser in the pipeline to resolve relative links into actual links, but it would be better to move to a permanent site such as linked.data.gov.au (see above). |
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.
@@ -83,7 +83,7 @@ class sum_of_earnings_in_last_52_weeks(Variable): | |||
value_type = float | |||
entity = Person | |||
label = u"Total earnings over last 52 weeks" | |||
reference = "http://organiccoders.allengeer.com/brk-1234-abcdfcrg234356/" | |||
reference = "https://github.com/OrganicCoders/openfisca-aotearoa/blob/master/openfisca_aotearoa/definitions/earnings_as_an_employee.md" |
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 not a stable URL: if the file gets renamed it would yield a 404. You can obtain a stable URL by navigating to this URL and pressing y
on your keyboard 🙂
OpenFisca Aotearoa (poly?) to ensure tests pass after OpenFisca Core upgrade
live_organ_donor_compensation
These changes:
These changes were made during the Better Rules Tech Week hackathon.