Skip to content

Conversation

@lars-reimann
Copy link
Contributor

@lars-reimann lars-reimann commented Sep 14, 2023

What problem do you want to solve?

As discussed in #601 (comment), this PR

  • renames hh_id to vg_id,
  • replaces the aggregation indicator _hh with _vg.

Since this PR touches thousands of files, I've split this from #601.

Todo

  • Pick an appropriate title.
  • Put Closes #XXXX in the first PR comment to auto-close the relevant issue once
    the PR is accepted. This is not applicable if there is no corresponding issue.
  • Document PR in CHANGES.md.

@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

Patch coverage: 97.70% and no project coverage change.

Comparison is base (affd33c) 90.61% compared to head (0303e2f) 90.61%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #654   +/-   ##
=======================================
  Coverage   90.61%   90.61%           
=======================================
  Files          48       48           
  Lines        3100     3100           
=======================================
  Hits         2809     2809           
  Misses        291      291           
Files Changed Coverage Δ
src/_gettsim/config.py 33.33% <ø> (ø)
src/_gettsim/taxes/lohn_st.py 86.07% <ø> (ø)
src/_gettsim/time_conversion.py 64.17% <ø> (ø)
...ettsim/transfers/kinderzuschl/kinderzuschl_eink.py 100.00% <ø> (ø)
src/_gettsim/transfers/kinderzuschl/kost_unterk.py 88.23% <60.00%> (ø)
.../_gettsim/transfers/arbeitsl_geld_2/kost_unterk.py 95.45% <92.30%> (ø)
src/_gettsim/aggregation_numpy.py 76.82% <100.00%> (ø)
src/_gettsim/demographic_vars.py 100.00% <100.00%> (ø)
src/_gettsim/interface.py 83.33% <100.00%> (ø)
src/_gettsim/synthetic.py 98.48% <100.00%> (ø)
... and 8 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lars-reimann lars-reimann marked this pull request as ready for review September 14, 2023 09:33
@hmgaudecker
Copy link
Collaborator

Thanks! Will need to alert some people before merging, but looks great at a glance!

Small things that I saw right away:

  • Please search for hh in the entire codebase -- mostly GEPs / docs / tutorials, where it still shows up
  • This will turn up haushaltsgröße_hhn in wohngeld.yaml. That key go completely, it is calculated as max_definierte_vg_größe.
  • We should document somewhere the origin of vg. Probably GEP-01 ?

@lars-reimann
Copy link
Contributor Author

We should document somewhere the origin of vg. Probably GEP-01 ?

What should be written there?

@hmgaudecker
Copy link
Collaborator

We should document somewhere the origin of vg. Probably GEP-01 ?

What should be written there?

Now that you are saying it, it is actually written already 🙈, right here... Apologies.

Reading through that makes me wonder whether this PR is actually helpful or not in its current form. That is, current hh will become either vg, or fg, or bg.

It might be more useful to use the correct naming already, leave all three of {vg, fg, bg} the same (=same behaviour as current hh) and then introduce the distinctions made in #601 ?

If that makes sense, it would be

  • vg in wohngeld
  • fg in elterngeld
  • bg in arbeitsl_geld_2, kinderzuschl

Does that make sense, @mjbloemer @ChristianZimpelmann @JuergenWiemers ?

@cpz-research
Copy link
Collaborator

Does that make sense,

I would say so!

Not really sure about grundrente. Probably fg? It could be the case even that we need another grouping there as I am not sure whether there is ever more than one generation considered.

A tricky part will be the Günstigerprüfung between Wohngeld and ALG II if those are referring to different groupings. But this isn't relevant in this PR.

@hmgaudecker hmgaudecker reopened this Sep 15, 2023
@lars-reimann
Copy link
Contributor Author

lars-reimann commented Nov 17, 2023

@hmgaudecker How should the following functions be renamed?

  • grunds_im_alter_m_hh in grunds_im_alter.py
  • grunds_im_alter_vermög_freib_hh in grunds_im_alter.py
  • wohngeld_kinderzuschl_vorrang_hh in benefit_checks.py

@hmgaudecker
Copy link
Collaborator

  • grunds_im_alter_m_vg in grunds_im_alter.py
  • grunds_im_alter_vermög_freib_vg in grunds_im_alter.py
  • wohngeld_kinderzuschl_vorrang_vg in benefit_checks.py

The last might actually pose a problem because wohngeld operates on different grouping than arbeitsl_geld_2, kinderzuschl, which we won't be able to solve before #601 and maybe a follow-up.

Just so you are not surprised.

@lars-reimann
Copy link
Contributor Author

Closing since main has already diverged too far from this.

@hmgaudecker hmgaudecker deleted the rename-hh_id-to-vg_id branch November 30, 2023 18:03
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.

4 participants