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

2021 Kinderzuschlag calculation #267

Merged
merged 16 commits into from Feb 22, 2021
Merged

2021 Kinderzuschlag calculation #267

merged 16 commits into from Feb 22, 2021

Conversation

Eric-Sommer
Copy link
Collaborator

@Eric-Sommer Eric-Sommer commented Feb 2, 2021

What problem do you want to solve?

Related to recent discussion on Zulip, this implements the kinderzuschlag calculation in place since 2021. This includes new parameters from Existenzminimumsberichte

  • redefine parent's share of living costs based, i.e. calculate directly from ExMin-Berichte
  • create new parameter kinderzuschlag_max, which is calculated from ExMin values starting in 2021.

Todo

  • Target the right branch and pick an appropriate title.
  • If it is a relevant contribution, put it in CHANGES.rst.
  • renamed kinderzuschlag_eink_max to kinderzuschlag_max_amount
  • introduce test. nothing fancy, just make sure that the theoretical maximum kinderzuschlag is 205€

@codecov
Copy link

codecov bot commented Feb 8, 2021

Codecov Report

Merging #267 (4e33b67) into master (118dd8b) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #267      +/-   ##
==========================================
+ Coverage   91.51%   91.54%   +0.02%     
==========================================
  Files          63       63              
  Lines        2298     2306       +8     
==========================================
+ Hits         2103     2111       +8     
  Misses        195      195              
Impacted Files Coverage Δ
...im/transfers/kinderzuschlag/kinderzuschlag_eink.py 84.21% <ø> (ø)
gettsim/policy_environment.py 100.00% <100.00%> (ø)
gettsim/tests/test_kinderzuschlag.py 100.00% <100.00%> (ø)
gettsim/transfers/kinderzuschlag/kost_unterk.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 118dd8b...80c5d10. Read the comment docs.

@Eric-Sommer Eric-Sommer marked this pull request as ready for review February 16, 2021 22:36
@@ -134,7 +134,7 @@ def anz_kinder_anspruch_per_hh(
return kindergeld_anspruch.groupby(hh_id).transform("sum")


def kinderzuschlag_eink_max(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am confused by name and docstring here. What exactly does the function return? Some income measure + the maximal child benefit surely is not the "maximal claim of child benefit" ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are rightly confused.

This is to determine the maximum income amount for which kiz is paid (before 2019). And it depends on the kiz itself. So the function name was not so bad, but the docstring was.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool, that makes sense!

@hmgaudecker
Copy link
Collaborator

Thanks! A couple of comments:

  1. I refactored things a little bit in set_up_policy_environment to reflect what the _parse_... functions are actually doing. We do not want to bloat them (e.g., anything that changes at some date into a function), I think, but have one function per concept.
  2. I am pretty sure we want to rename params["kinderzuschlag"]["kinderzuschlag"] to params["kinderzuschlag"]["kinderzuschlag_max"] pre-2020 already in the YAML files. The current name is just confusing and it is renamed immediately, I do not think we ever use the "kinderzuschlag" value. Thoughts?
  3. If yes on 2., we should put if date.year >= 2021 outside the function _parse_kinderzuschlag_max and just call it with the params argument.
  4. The actual calculation still has a confusing part in it, see my comment there.

Copy link
Collaborator

@hmgaudecker hmgaudecker left a comment

Choose a reason for hiding this comment

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

I am sorry, @ChristianZimpelmann, but this is definitely not ready to go yet. Se my previous comments.

@Eric-Sommer
Copy link
Collaborator Author

Eric-Sommer commented Feb 21, 2021

2. I am pretty sure we want to rename `params["kinderzuschlag"]["kinderzuschlag"]` to `params["kinderzuschlag"]["kinderzuschlag_max"]` pre-2020 already in the YAML files. The current name is just confusing and it is renamed immediately, I do not think we ever use the `"kinderzuschlag"` value. Thoughts?

I agree. It has always been the maximum amount.

3. If yes on 2., we should put `if date.year >= 2021` outside the function `_parse_kinderzuschlag_max` and just call it with the `params` argument.

OK, that would spare a few lines of code

@hmgaudecker
Copy link
Collaborator

hmgaudecker commented Feb 21, 2021

  1. If yes on 2., we should put if date.year >= 2021 outside the function _parse_kinderzuschlag_max and just call it with the params argument.

OK, that would spare a few lines of code

I think it just makes it clearer given the way it is structured, but this is a very minor thing.

@Eric-Sommer
Copy link
Collaborator Author

I think it just makes it clearer given the way it is structured, but this is a very minor thing.

I agree. It is a minor thing for this PR, but a good general idea if this occurs more often in the future.

@hmgaudecker
Copy link
Collaborator

I think it just makes it clearer given the way it is structured, but this is a very minor thing.

I agree. It is a minor thing for this PR, but a good general idea if this occurs more often in the future.

It will be a case-by-case basis, often we'll want it inside the function. Here, the thing is that prior to 2021, it is directly a parameter. We should note that in the yaml file, btw. Do we set things to missing if a parameter becomes irrelevant? I do not remember right now...

@Eric-Sommer
Copy link
Collaborator Author

I think it just makes it clearer given the way it is structured, but this is a very minor thing.

I agree. It is a minor thing for this PR, but a good general idea if this occurs more often in the future.

It will be a case-by-case basis, often we'll want it inside the function. Here, the thing is that prior to 2021, it is directly a parameter. We should note that in the yaml file, btw. Do we set things to missing if a parameter becomes irrelevant? I do not remember right now...

There is no standard procedure. Usually, if a parameter becomes irrelevant, this means that we need to implement a new function, thereby implicitly rendering a parameter irrelevant. Adding a note in the yaml file makes certainly sense

Copy link
Collaborator

@hmgaudecker hmgaudecker left a comment

Choose a reason for hiding this comment

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

Thanks for all your work yesterday, looks great now! I just made a check clearer and updated a couple of docstrings.

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

3 participants