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

[WIP] Add organ donor compensation #16

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

MattiSG
Copy link
Member

@MattiSG MattiSG commented May 20, 2018

  • Tax and benefit system evolution.
  • Impacted periods: all.
  • Impacted areas: live_organ_donor_compensation
  • Details:
    • Add computation for live organ donor compensation.

These changes:

  • Add a new feature (for instance adding a variable)

These changes were made during the Better Rules Tech Week hackathon.

Copy link
Member Author

@MattiSG MattiSG left a 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"
Copy link
Member Author

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 🙂

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"
Copy link
Member Author

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
Copy link
Member Author

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.

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.

@MattiSG
Copy link
Member Author

MattiSG commented May 21, 2018

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 🙂

@MattiSG MattiSG requested a review from verbman May 21, 2018 05:03
Copy link

@nigelcharman nigelcharman left a 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

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"

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

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 "
Copy link
Member

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

Choose a reason for hiding this comment

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

Fixed, thanks

@MattiSG
Copy link
Member Author

MattiSG commented May 22, 2018

Thanks a lot for all these changes already! 👏

where these might transition to?

The best would probably be legislation.govt.nz 🙂 If there is no better reference, this is still much better than nothing!

it is far from complete in terms of the legislation

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.

@BrigetteM
Copy link

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.

@nigelcharman
Copy link

Thanks @BrigetteM. I can see how linked.data.gov.au would help with this. Hope you've shown that to Pia and co.
In the meantime, to preserve the work you did on the linked data definitions for this project, I've added to OrganicCoders#4. As noted on the PR, it's not optimal since the URL contains the repository and branch name, so I see this as a step towards a more permanent home.

@nigelcharman
Copy link

I've updated this PR with:

  • New definitions folder containing definitions for 'number of weeks' and 'earnings as an employee'
  • Updated the references in variables/live_organ_donor_compensation.py to reference these definitions

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

Copy link
Member Author

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

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

The idea of adding “definitions” straight in the OpenFisca is very thought-provoking. I find it very interesting, but I feel that it also bring quite a commitment to maintain them.
@verbman @Br3nda what do you think of that? Should we bring an ontology into OpenFisca Aotearoa?

@@ -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"
Copy link
Member Author

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 🙂

@Br3nda Br3nda changed the title Add organ donor compensation [WIP] Add organ donor compensation Aug 12, 2018
@verbman verbman removed their request for review October 8, 2018 03:32
@Br3nda Br3nda added this to Inbox in OpenFisca Aotearoa Apr 17, 2019
@mermop mermop removed this from Inbox in OpenFisca Aotearoa Jun 22, 2020
verbman added a commit that referenced this pull request May 7, 2024
OpenFisca Aotearoa (poly?) to ensure tests pass after OpenFisca Core upgrade
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.

None yet

5 participants