iza-institute-of-labor-economics / gettsim

The GErman Taxes and Transfers SIMulator
https://gettsim.readthedocs.io/
GNU Affero General Public License v3.0
56 stars 33 forks source link

Remove tax unit groupings. #694

Closed MImmesberger closed 8 months ago

MImmesberger commented 8 months ago

Closes #690

lars-reimann commented 8 months ago

Regarding this comment:

@lars-reimann Can you have a look where the CondaVerificationError in the readthedocs check comes from?

I could not reproduce this locally. It seems to be caused by the upgrade of sphinx-automodapi to version 0.17.0. In previous, successful runs version 0.16.0 was used. Maybe you can add an upper bound for sphinx-automodapi in environment.yml in this PR and try again?

- sphinx-automodapi<0.17.0
lars-reimann commented 8 months ago

That seems to have fixed the install issue at least.

MImmesberger commented 8 months ago

Yes, I guess the other errors are due to this incomplete PR. Thanks!

MImmesberger commented 8 months ago

Note: Currently, we need to specifiy p_id_ehepartner and p_id_einstandspartner separately. Might make sense to set p_id_einstandspartner automatically if p_id_ehepartner is specified.

hmgaudecker commented 8 months ago

I don't see a point in that. May save a user a line of code and create confusion if we silently modify one of her variables.

Still, very good point! Just a different conclusion: please add a data consistency check on this.

MImmesberger commented 8 months ago

Some things that I have encountered:

  1. I have renamed eigener_bedarf_gedeckt to eigenbedarf_gedeckt to adhere with the naming rules.
  2. p_id_einstandspartner is 1 character too long. Maybe rename all of these grouping pointers to pid_...? Or go with p_id_ep?
  3. Betreuungskosten are now aggregated via p_id. Currently I use the user-specified Betreuungskosten-Träger ID p_id_betreuungsk_trä (at the character limit) as the p_id_to_aggregate_by. Alternatively, we could use p_id_kindergeld_empf.
  4. The Wohngeld Vorrang check in wohngeld_vorrang_hh is wrong (as previously discussed, I added a ToDo for that that links to #690 ).

Additionally two questions regarding Grundsicherung im Alter:

  1. Currently, Grundsicherung im Alter is only payed out if all individuals in the household are retired. There is a ToDo without a linked issue. I guess we don't touch this part in this PR?
  2. grunds_im_alter_m_eg uses arbeitsl_geld_2_regelbedarf_m_bg as relevant income. I don't know whether the correct grouping should be arbeitsl_geld_2_regelbedarf_m_eg, but then again, there is the question on how to split up the ALG2 benefits.
hmgaudecker commented 8 months ago

I have renamed eigener_bedarf_gedeckt to eigenbedarf_gedeckt to adhere with the naming rules.

Is that the correct term from the law / commonplace? Sounds funny, but that does not mean anything.

hmgaudecker commented 8 months ago
  • p_id_einstandspartner is 1 character too long. Maybe rename all of these grouping pointers to pid_...? Or go with p_id_ep?

Let's just disable the test. Change is around the corner and we should not spend time on this anymore.

hmgaudecker commented 8 months ago
  • Betreuungskosten are now aggregated via p_id. Currently I use the user-specified Betreuungskosten-Träger ID p_id_betreuungsk_trä (at the character limit) as the p_id_to_aggregate_by. Alternatively, we could use p_id_kindergeld_empf.

See previous thing, just go to p_id_betreuungsk_träger. But we probably won't loose much if using the Kindergeld version? I'd probably favor the latter but that'll be your call.

hmgaudecker commented 8 months ago
  1. Currently, Grundsicherung im Alter is only payed out if all individuals in the household are retired. There is a ToDo without a linked issue. I guess we don't touch this part in this PR?

No, that can remain a to-do, this PR is humongous already. Maybe @ChristianZimpelmann remembers the background?

  1. grunds_im_alter_m_eg uses arbeitsl_geld_2_regelbedarf_m_bg as relevant income. I don't know whether the correct grouping should be arbeitsl_geld_2_regelbedarf_m_eg, but then again, there is the question on how to split up the ALG2 benefits.

We might have to repeat those calculations, unfortunately. But if you can find a workaround for now, no need to solve it at this very moment!

MImmesberger commented 8 months ago

Is that the correct term from the law / commonplace? Sounds funny, but that does not mean anything.

Yes, the Arbeitsagentur uses it

MImmesberger commented 8 months ago

We might have to repeat those calculations, unfortunately. But if you can find a workaround for now, no need to solve it at this very moment!

The current implementation may be a bad approximation if there are retirees that have children in their Bedarfsgemeinschaft. Even then, the current implementation might be correct, but I couldn't find anything online. But I think this is not too common, so we can ignore it for now.

codecov[bot] commented 8 months ago

Codecov Report

Attention: Patch coverage is 97.22222% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 89.49%. Comparing base (07baf57) to head (849ba6a).

Files Patch % Lines
src/_gettsim/groupings.py 93.75% 2 Missing :warning:
src/_gettsim/taxes/eink_st.py 93.93% 2 Missing :warning:
src/_gettsim/functions_loader.py 0.00% 1 Missing :warning:
.../_gettsim/transfers/arbeitsl_geld_2/kost_unterk.py 75.00% 1 Missing :warning:
src/_gettsim/transfers/kinderzuschl/kost_unterk.py 91.66% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #694 +/- ## ========================================== + Coverage 89.22% 89.49% +0.27% ========================================== Files 51 51 Lines 3592 3637 +45 ========================================== + Hits 3205 3255 +50 + Misses 387 382 -5 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ChristianZimpelmann commented 8 months ago

I think this PR could also close #89

  1. Currently, Grundsicherung im Alter is only payed out if all individuals in the household are retired. There is a ToDo without a linked issue. I guess we don't touch this part in this PR?

We were not sure how to handle cases in which one person in the Bedarfsgemeinschaft is retired, but another person is unemployed and searching for a job. We opted for this simplified version, but we should reconsider at some point. I now opened an issue for it: #703 Can you link it in the code?

MImmesberger commented 8 months ago

The tax unit grouping is now removed in the code and any mention of it in the docs is gone.

I slightly adjusted the tutorials by manually grouping the GETTSIM outputs on the hh level where tu groupings were used before. I think this keeps the tutorial as simple as possible.

MImmesberger commented 8 months ago

I don't think there are particular tests you have to look at.

Just so it doesn't sneak through the review process: In 1d78ded, I set the minimum relevant income for ALG2 to 0. There were some test cases that resulted in negative values after getting rid of tu.

MImmesberger commented 8 months ago

The rounding seems to be an issue when switching from eink_st_y_sn / 12 to eink_st_m_sn, because rounding parameters are expected for the eink_st_m_sn key in the param yaml. They are specified for the eink_st_y_sn key only.

I'm not entirely sure what the expected behavior should be:

  1. First round and then time convert (with optional rounding spec for converted column).
  2. First time convert and then round (We could use a quick fix by specifying the eink_st_m_sn rounding spec in the params yaml).

Because eink_st_m_sn is used as an approximation for Lohnsteuer (at least for Elterngeld and Grundsicherung; not sure about ALG2) we cannot infer expected behavior from the law.

hmgaudecker commented 8 months ago

Just so it doesn't sneak through the review process: In 1d78ded, I set the minimum relevant income for ALG2 to 0. There were some test cases that resulted in negative values after getting rid of tu.

Cool, thanks for the pointer. In principle, I think that ALG II should guarantee a consumption floor, but it might be that the mechanism for that is more complicated. I would have no idea. But I'd kind of like to guarantee that subsistence level (thinking of running GETTSIM on models that do require a positive consumption), unless the law is different.

If possible, I'd prefer to revert and make an issue that we'll need to investigate this case.

hmgaudecker commented 8 months ago

The rounding seems to be an issue when switching from eink_st_y_sn / 12 to eink_st_m_sn, because rounding parameters are expected for the eink_st_m_sn key in the param yaml. They are specified for the eink_st_y_sn key only.

I'm not entirely sure what the expected behavior should be:

1. First round and then time convert (with optional rounding spec for converted column).

2. First time convert and then round (We could use a quick fix by specifying the `eink_st_m_sn` rounding spec in the params yaml).

Because eink_st_m_sn is used as an approximation for Lohnsteuer (at least for Elterngeld and Grundsicherung; not sure about ALG2) we cannot infer expected behavior from the law.

Behavior 1. and the last two cases we should get rid of eventually now that we have Lohnsteuer.

MImmesberger commented 8 months ago

The rounding seems to be an issue when switching from eink_st_y_sn / 12 to eink_st_m_sn, because rounding parameters are expected for the eink_st_m_sn key in the param yaml. They are specified for the eink_st_y_sn key only. I'm not entirely sure what the expected behavior should be:

1. First round and then time convert (with optional rounding spec for converted column).

2. First time convert and then round (We could use a quick fix by specifying the `eink_st_m_sn` rounding spec in the params yaml).

Because eink_st_m_sn is used as an approximation for Lohnsteuer (at least for Elterngeld and Grundsicherung; not sure about ALG2) we cannot infer expected behavior from the law.

Behavior 1. and the last two cases we should get rid of eventually now that we have Lohnsteuer.

@lars-reimann Can you have a look?

hmgaudecker commented 8 months ago

@lars-reimann Can you have a look?

Would be great, but let's do that separately from here. Apologies if that means reverting something I noted. Also fine to skip a couple of tests for the moment.

MImmesberger commented 8 months ago

Cool, thanks for the pointer. In principle, I think that ALG II should guarantee a consumption floor, but it might be that the mechanism for that is more complicated. I would have no idea. But I'd kind of like to guarantee that subsistence level (thinking of running GETTSIM on models that do require a positive consumption), unless the law is different.

I think we are talking about different things here. I realized that the issue I raised is solved once we fix #606 because it makes sure that we don't compute negative "anrechenbares Erwerbseinkommen" on the individual level.